On Tue, Feb 16, 2021 at 6:33 PM Peter Krempa <pkre...@redhat.com> wrote:
> 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. > > 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 -- Marc-André Lureau
