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

[Qemu-devel] [PATCH v2] 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. 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)) {
-- 
2.9.3

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

[Markus Armbruster]

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.

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.

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[**].


[*] 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.

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

[Daniel Henrique Barboza]

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

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

[Juan Quintela]

Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> wrote:
> 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.

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.

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

[Markus Armbruster]

Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> writes:

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

Yes, QMP migrate_set_downtime takes seconds, but I'm talking about QMP
command migrate-set-parameters, which takes milliseconds.

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

That's reasonable!  The problem is that you fix one error message by
breaking another one.

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

Changing QMP commands incompatibly is a big no-no.

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

I think that's a fine reason for deprecating the migrate_set_downtime
command, and ask people to use migrate-set-parameters instead.

>> [*] 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.
>>

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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> writes:
> 
> > 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().
> 
> Yes, QMP migrate_set_downtime takes seconds, but I'm talking about QMP
> command migrate-set-parameters, which takes milliseconds.
> 
> >> 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,
> 
> That's reasonable!  The problem is that you fix one error message by
> breaking another one.
> 
> > 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.
> 
> Changing QMP commands incompatibly is a big no-no.
> 
> > 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.
> 
> I think that's a fine reason for deprecating the migrate_set_downtime
> command, and ask people to use migrate-set-parameters instead.

OK, but you can set it to 300 from migrate_set_downtime by doing
HMP:
   migrate_set_downtime 0.3 
QMP:
   { "execute": "migrate_set_downtime", "arguments": { "value": 0.3 } }

so it's bogus to say that's a reason to ditch it.

This has all got rather complex for a simple patch to change an error
message.  Can't we just come up with a clear error message that works in
all cases.

Dave

> >> [*] 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.
> >>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> writes:
>> 
>> > 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().
>> 
>> Yes, QMP migrate_set_downtime takes seconds, but I'm talking about QMP
>> command migrate-set-parameters, which takes milliseconds.
>> 
>> >> 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,
>> 
>> That's reasonable!  The problem is that you fix one error message by
>> breaking another one.
>> 
>> > 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.
>> 
>> Changing QMP commands incompatibly is a big no-no.
>> 
>> > 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.
>> 
>> I think that's a fine reason for deprecating the migrate_set_downtime
>> command, and ask people to use migrate-set-parameters instead.
>
> OK, but you can set it to 300 from migrate_set_downtime by doing
> HMP:
>    migrate_set_downtime 0.3 
> QMP:
>    { "execute": "migrate_set_downtime", "arguments": { "value": 0.3 } }
>
> so it's bogus to say that's a reason to ditch it.

I bought Daniel's assertion without checking it.  Thanks for the
correction.

> This has all got rather complex for a simple patch to change an error
> message.  Can't we just come up with a clear error message that works in
> all cases.

I outlined how to fix migrate_set_downtime's error message without
breaking migrate-set-parameters's in my review.

If we can come up with a single message that works for both commands,
even better.

>
> Dave
>
>> >> [*] 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.
>> >>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel Henrique Barboza]

On 02/21/2017 12:47 PM, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> writes:
>>>
>>>> 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().
>>> Yes, QMP migrate_set_downtime takes seconds, but I'm talking about QMP
>>> command migrate-set-parameters, which takes milliseconds.
>>>
>>>>> 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,
>>> That's reasonable!  The problem is that you fix one error message by
>>> breaking another one.

I see. The problem is that I haven't taken into consideration that 
set_downtime
can also be set by migrate_set_parameters. My bad.

I'll fix the error message of set_downtime without breaking the one in
set_parameters in v3.


Daniel
>>>
>>>> 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.
>>> Changing QMP commands incompatibly is a big no-no.
>>>
>>>> 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.
>>> I think that's a fine reason for deprecating the migrate_set_downtime
>>> command, and ask people to use migrate-set-parameters instead.
>> OK, but you can set it to 300 from migrate_set_downtime by doing
>> HMP:
>>     migrate_set_downtime 0.3
>> QMP:
>>     { "execute": "migrate_set_downtime", "arguments": { "value": 0.3 } }
>>
>> so it's bogus to say that's a reason to ditch it.
> I bought Daniel's assertion without checking it.  Thanks for the
> correction.
>
>> This has all got rather complex for a simple patch to change an error
>> message.  Can't we just come up with a clear error message that works in
>> all cases.
> I outlined how to fix migrate_set_downtime's error message without
> breaking migrate-set-parameters's in my review.
>
> If we can come up with a single message that works for both commands,
> even better.
>
>> Dave
>>
>>>>> [*] 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.
>>>>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK