Re: [Qemu-devel] [PATCH v2] Changing error message of QMP 'migrate_set_downtime' to seconds

[Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>]

On 02/21/2017 06:02 AM, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> writes:
>
>> The previous error message was displaying the values in miliseconds,
>> being misleading with the command that accepts the value in seconds:
>>
>> { "execute": "migrate_set_downtime", "arguments": {"value": 3000}}
>> {"error": {"class": "GenericError", "desc": "Parameter 'downtime_limit'
>> expects an integer in the range of 0 to 2000000 milliseconds"}}
>>
>> This patch changes it to '2000 seconds' to keep consistency with
>> the expected parameter. The macro 'QERR_INVALID_PARAMETER_VALUE'
>> was changed for a regular string that allows the use of the
>> MAX_MIGRATE_SET_DOWNTIME as a parameter, instead of hardcoding
>> the value in the error message.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> ---
>>   migration/migration.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index c6ae69d..c05e764 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -49,6 +49,9 @@
>>    * for sending the last part */
>>   #define DEFAULT_MIGRATE_SET_DOWNTIME 300
>>   
>> +/* Maximum migrate downtime set to 2000*1000 miliseconds */
>> +#define MAX_MIGRATE_SET_DOWNTIME (2000 * 1000)
>> +
>>   /* Default compression thread count */
>>   #define DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT 8
>>   /* Default decompression thread count, usually decompression is at
>> @@ -843,10 +846,11 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>           return;
>>       }
>>       if (params->has_downtime_limit &&
>> -        (params->downtime_limit < 0 || params->downtime_limit > 2000000)) {
>> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> -                   "downtime_limit",
>> -                   "an integer in the range of 0 to 2000000 milliseconds");
>> +        (params->downtime_limit < 0 ||
>> +         params->downtime_limit > MAX_MIGRATE_SET_DOWNTIME)) {
>> +        error_setg(errp, "Parameter 'downtime_limit' expects an integer in "
>> +                         "the range of 0 to %d seconds",
>> +                         MAX_MIGRATE_SET_DOWNTIME / 1000);
>>           return;
>>       }
>>       if (params->has_x_checkpoint_delay && (params->x_checkpoint_delay < 0)) {
> Isn't this wrong for QMP migrate-set-parameters?  There, the unit is
> milliseconds, i.e. the new error message is as wrong as the old one is
> for migrate_set_downtime.

Actually the unit for migrate-set-parameters, as seen by the caller,
is seconds. The underlying logic receives the input and multiplies it
by 1000 in qmp_migrate_set_downtime().
>
> I'm afraid you need to fix the error message in
> qmp_migrate_set_downtime().  If you assume qmp_migrate_set_parameters()
> fails only in one way, replace its error object by one with a better
> message[*].  If you'd rather not assume, you need to refactor things so
> that each place can set the downtime and create an appropriate error on
> failure.

There is at least one similar usage of this error message just
above this code in max_bandwidth param. Perhaps a new
function/macro to deal with these cases is justified.

>
> We might want to check other command wrappers that translate units.
>
> Time units are a hopeless mess in QMP.  We should've enforced uniform
> usage of either seconds or nanoseconds.  The latter to placate
> irrational fear of floating-point[**].

I agree that my patch doesn't make it much better. I just set the
error message to be in seconds to be consistent with the user input,
but the code now feels 'awkward' when you do a verification
in milliseconds and deliver the error message in seconds.

One thing that can be done is to make migrate-set-downtime to
accept milliseconds instead of seconds. I wasn't willing at first to
change the migrate-set-downtime API because of an error
message, however it really feels like the right thing to do here.
Specially when you consider that the default value of this parameter,
set by DEFAULT_MIGRATE_SET_DOWNTIME, is 300 - a value that
in theory the user shouldn't be able to set in the API.




Daniel

>
>
> [*] If replacing messages turns out to be a common operation, we can add
> a function to replace it within the same Error object.
>
> [**] If you think the rounding you can get when converting
> floating-point seconds to integer milli-, micro- or nanoseconds matters,
> I have time-keeping equipment to sell you.
>