Re: [Qemu-devel] [PATCH v3 18/22] qcow2: Use intermediate helper CB for amend

[Stefan Hajnoczi]

On Thu, Nov 20, 2014 at 06:06:34PM +0100, Max Reitz wrote:
> @@ -2674,6 +2743,7 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>      bool encrypt;
>      int ret;
>      QemuOptDesc *desc = opts->list->desc;
> +    Qcow2AmendHelperCBInfo helper_cb_info;
>  
>      while (desc && desc->name) {
>          if (!qemu_opt_find(opts, desc->name)) {
> @@ -2731,6 +2801,12 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>          desc++;
>      }
>  
> +    helper_cb_info = (Qcow2AmendHelperCBInfo){
> +        .original_status_cb = status_cb,
> +        .original_cb_opaque = cb_opaque,
> +        .total_operations = (new_version < old_version)

If you respin, another way of writing this is without total_operations
here (so it initializes to 0)...

> +    };
> +
>      /* Upgrade first (some features may require compat=1.1) */
>      if (new_version > old_version) {
>          s->qcow_version = new_version;
> @@ -2789,7 +2865,9 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>  
>      /* Downgrade last (so unsupported features can be removed before) */
>      if (new_version < old_version) {
> -        ret = qcow2_downgrade(bs, new_version, status_cb, cb_opaque);
> +        helper_cb_info.current_operation = QCOW2_DOWNGRADING;

...and then helper_cb_info.total_operations++ here.

That way the new_version < old_version check is not duplicated into the
helper_cb_info initializer.

The code is clearer because we assign current_operation and
total_operations at the same time.

Just a style suggestion, feel free to ignore if you don't like it.

pgpsX0k4CDoqi.pgp
Description: PGP signature