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" <address@hidden> writes:
>
> > * Markus Armbruster (address@hidden) wrote:
> > > Daniel Henrique Barboza <address@hidden> writes:
> > >
> > > > 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().
> > > 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 / address@hidden / Manchester, UK