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.
Sure, although it's an internal API, I agree it's error prone.
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.
Let's make it an enum for the reason.
thanks