Re: [Qemu-block] [PATCH for-2.10 06/10] hmp: Clean up and simplify hmp_migrate_set_parameter()

[Eric Blake]

On 07/18/2017 08:41 AM, Markus Armbruster wrote:
> The bulk of hmp_migrate_set_parameter()'s code sets one member of
> MigrationParameters according to the command's arguments.  It uses a
> string visitor for integer and boolean members, but not for string and
> size members.  It calls visit_type_bool() right away, but delays
> visit_type_int() some.  The delaying requires a flag variable and a
> bit of trickery: we set all integer members instead of just the one we
> want, and rely on the has_FOOs to mask the unwanted ones.
> 
> Clean this up as follows.  Don't delay calling visit_type_int().  Use
> the string visitor for strings, too.  This involves extra allocations
> and cleanup, but doing them is simpler and cleaner than avoiding them.
> 
> Sadly, using the string visitor for sizes isn't possible, because it
> defaults to Bytes rather than Mebibytes.  Add a comment explaining
> that.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hmp.c | 78 
> ++++++++++++++++++++++++++++---------------------------------------
>  1 file changed, 32 insertions(+), 46 deletions(-)
>

Diffstat says it's cleaner.

>              case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> -                p.has_max_bandwidth = true;
> +                p->has_max_bandwidth = true;
> +                /*
> +                 * Can't use visit_type_size() here, because it
> +                 * defaults to Bytes rather than Mebibytes.
> +                 */
>                  ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);

Can we munge valuestr (if it has a suffix, leave it alone; if it has
none, tack on "M"), and then call the size visitor as usual?

>                  if (ret < 0 || valuebw > INT64_MAX
>                      || (size_t)valuebw != valuebw) {
>                      error_setg(&err, "Invalid size %s", valuestr);
> -                    goto cleanup;
> +                    break;
>                  }

But for now, the inline version works.

> -                /* Set all integers; only one has_FOO will be set, and
> -                 * the code ignores the remaining values */
> -                p.compress_level = valueint;
> -                p.compress_threads = valueint;
> -                p.decompress_threads = valueint;
> -                p.cpu_throttle_initial = valueint;
> -                p.cpu_throttle_increment = valueint;
> -                p.downtime_limit = valueint;
> -                p.x_checkpoint_delay = valueint;
> +            if (err) {
> +                goto cleanup;
>              }
>  
> -            qmp_migrate_set_parameters(&p, &err);

The old code is calling qmp_migrate_set_parameters() once per loop
iteration, because it could only parse one integer per loop.

> +            qmp_migrate_set_parameters(p, &err);

But couldn't you now sink this below the loop, and call it only once
after all members are parsed?  Can be a separate patch, though.

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature