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

[Markus Armbruster]

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.

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.