| Message ID | 20170220160316.16803-1-danielhb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. >
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.
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. >>
* 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
"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
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
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)) {
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(-)