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

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

On 02/17/2017 03:37 PM, Paolo Bonzini wrote:
>
> On 17/02/2017 18:26, Daniel Henrique Barboza wrote:
>> 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.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> ---
>>   migration/migration.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index c6ae69d..2dc63b1 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)) {
>> +        (params->downtime_limit < 0 ||
>> +         params->downtime_limit > MAX_MIGRATE_SET_DOWNTIME)) {
>>           error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>                      "downtime_limit",
>> -                   "an integer in the range of 0 to 2000000 milliseconds");
>> +                   "an integer in the range of 0 to 2000 seconds");
> Perhaps you could use %d and set  MAX_MIGRATE_SET_DOWNTIME to 2000?
> Though perhaps the migration maintainers are okay with the patch as is.

I did that at first but I got errors on "error_setg" about the extra 
parameter.
I even considered using sprintf to format the string but I was afraid it 
would be
a little overkill.


Daniel

>
> Paolo
>
>>           return;
>>       }
>>       if (params->has_x_checkpoint_delay && (params->x_checkpoint_delay < 0)) {
>>