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