On Tue, Feb 16, 2021 at 17:38:37 +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau <marcandre.lur...@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.lur...@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.