[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] qga: return a more explicit error on why a command is disabl
From: |
Peter Krempa |
Subject: |
Re: [PATCH] qga: return a more explicit error on why a command is disabled |
Date: |
Tue, 16 Feb 2021 15:30:27 +0100 |
User-agent: |
Mutt/1.14.6 (2020-07-11) |
On Tue, Feb 16, 2021 at 17:38:37 +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qmp_disable_command() now takes an optional error string to return a
> more explicit error message.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1928806
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/qapi/qmp/dispatch.h | 3 ++-
> qapi/qmp-dispatch.c | 2 +-
> qapi/qmp-registry.c | 9 +++++----
> qga/main.c | 4 ++--
> 4 files changed, 10 insertions(+), 8 deletions(-)
[...]
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 0a2b20a4e4..ce4bf56b2c 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -157,7 +157,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
> *request,
> }
> if (!cmd->enabled) {
> error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> - "The command %s has been disabled for this instance",
> + cmd->err_msg ?: "The command %s has been disabled for this
> instance",
Passing in the formatting string from a variable looks shady and it's
hard to ensure that callers pass in the appropriate format modifier ...
> command);
> goto out;
> }
[...]
> diff --git a/qga/main.c b/qga/main.c
> index e7f8f3b161..c59b610691 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -375,7 +375,7 @@ static void ga_disable_non_whitelisted(const QmpCommand
> *cmd, void *opaque)
> }
> if (!whitelisted) {
> g_debug("disabling command: %s", name);
> - qmp_disable_command(&ga_commands, name);
> + qmp_disable_command(&ga_commands, name, "The command was disabled
> after fsfreeze.");
... such as here where '%s' is missing. Luckily that is not a problem
compared to when there would be more than one format modifier.
But the error message looks definitely better.
> }
> }
My suggestion would be to store a boolean flag that the command was
disabled due to an ongoing fsfreeze and then choose the appropriate
error message directly at the point where it's reported.