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

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

[Daniel Henrique Barboza]

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");
         return;
     }
     if (params->has_x_checkpoint_delay && (params->x_checkpoint_delay < 0)) {
-- 
2.9.3

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

[Paolo Bonzini]

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.

Paolo

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

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

[Daniel Henrique Barboza]

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)) {
>>

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

On 02/17/2017 01:01 PM, Daniel Henrique Barboza wrote:

>>> 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.

Ah, right, because QERR_INVALID_PARAMETER_VALUE is a macro that expands
to a fixed printf-style format string where you have to know how many
exact arguments it further expects.  The only way around that is to
open-code the error message you want, instead of forcing the use of the
awkward macro.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/17/2017 01:01 PM, Daniel Henrique Barboza wrote:
>
>>>> 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.
>
> Ah, right, because QERR_INVALID_PARAMETER_VALUE is a macro that expands
> to a fixed printf-style format string where you have to know how many
> exact arguments it further expects.  The only way around that is to
> open-code the error message you want, instead of forcing the use of the
> awkward macro.

Go ahead and open-code whenever that results in better error messages.