|
From: | Daniel Henrique Barboza |
Subject: | Re: [Qemu-devel] [PATCH v2] Changing error message of QMP 'migrate_set_downtime' to seconds |
Date: | Tue, 21 Feb 2017 10:31:25 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
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().
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.
[Prev in Thread] | Current Thread | [Next in Thread] |