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

[Juan Quintela]

Daniel Henrique Barboza <address@hidden> wrote:
> On 02/21/2017 06:02 AM, Markus Armbruster wrote:
>> Daniel Henrique Barboza <address@hidden> 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 <address@hidden>
>>> ---
>>>   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.

libvirt and users use the API, I don't see how to change it in an easy
way :-(

I think that the better way we can do is change the error message and be
done with it.

Later, Juan.