Re: [Qemu-devel] [PATCH v6] migrate: move max-bandwidth and downtime-limit to migrate_set_parameter

[Ashijeet Acharya]

On Thu, Sep 15, 2016 at 9:18 PM, Eric Blake <address@hidden> wrote:
> On 09/14/2016 12:40 PM, Ashijeet Acharya wrote:
>> Mark the old commands 'migrate_set_speed' and 'migrate_set_downtime' as
>> deprecated.
>> Move max-bandwidth and downtime-limit into migrate-set-parameters for
>> setting maximum migration speed and expected downtime limit parameters
>> respectively.
>> Change downtime units to milliseconds (only for new-command) and set
>> its upper bound limit to 2000 seconds.
>> Update the query part in both hmp and qmp qemu control interfaces.
>>
>> Signed-off-by: Ashijeet Acharya <address@hidden>
>> ---
>
>> +++ b/migration/migration.c
>
>> @@ -804,6 +802,20 @@ void qmp_migrate_set_parameters(MigrationParameters 
>> *params, Error **errp)
>>                     "cpu_throttle_increment",
>>                     "an integer in the range of 1 to 99");
>>      }
>> +    if (params->has_max_bandwidth &&
>> +        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE "%zu",
>> +                   "max_bandwidth",
>> +                   "an integer in the range of 0 to ", SIZE_MAX);
>
> That looks weird (not necessarily your fault; but it explains why we've
> been trying to avoid new QERR_ macros).  No other use of
> QERR_INVALID_PARAMETER_VALUE adds additional text.  So I'm thinking it's
> better to just follow suit with a shorter non-parameterized message;
> such as in this instance:
>
> balloon.c:        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "target", "a size");
>
> Or, just open-code the full error message, instead of having half the
> message hidden behind the QERR_INVALID_PARAMETER_VALUE macro.

Alright, I will open-code the error message completely and drop the
QERR_INVALID_PARAMETER_VALUE macro.
That will look much cleaner.

>
> Otherwise, it looks good to me.  With the error message sorted out, you
> can add:
>
> Reviewed-by: Eric Blake <address@hidden>

Okay, I will sort out the error message and submit v7 along with your R-b tag.

Ashijeet
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>