On Tue, Feb 16, 2021 at 6:33 PM Peter Krempa <pkrempa@redhat.com> wrote:

> 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


-- 
Marc-André Lureau