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 <arm...@redhat.com> > --- > 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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
