Eric Blake <ebl...@redhat.com> writes:
> 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?
We could, but I expect it to be more complicated than this inline copy.
A possible alternative is adding flags to string_input_visitor_new(). I
suspect that will also be more complicated once you add tests. Keeping
non-standard suffixes somewhat bothersome will at least help prevent
more of them.
>> 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.
The whole switch could sink along. Left for another day.
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks!
