Hi On Thu, Feb 18, 2021 at 8:40 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > > > qmp_disable_command() now takes an enum for the reason, to be able > > to give more explicit error messages. > > > > Fixes: > > https://bugzilla.redhat.com/show_bug.cgi?id=1928806 > > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > --- > > > > v2: > > - replace string with an enum for disabling reason > > - remove trailing dot > > > > include/qapi/qmp/dispatch.h | 12 ++++++++++-- > > monitor/qmp-cmds-control.c | 2 +- > > qapi/qmp-dispatch.c | 10 +++++++++- > > qapi/qmp-registry.c | 16 +++++++++------- > > qga/main.c | 4 ++-- > > 5 files changed, 31 insertions(+), 13 deletions(-) > > > > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h > > index 1486cac3ef..fda9ffad73 100644 > > --- a/include/qapi/qmp/dispatch.h > > +++ b/include/qapi/qmp/dispatch.h > > @@ -28,6 +28,13 @@ typedef enum QmpCommandOptions > > QCO_COROUTINE = (1U << 3), > > } QmpCommandOptions; > > > > +typedef enum QmpDisabled > > +{ > > + QMP_DISABLED_NONE, > > + QMP_DISABLED_GENERIC, > > + QMP_DISABLED_FROZEN, > > +} QmpDisabled; > > + > > I strongly dislike baking QGA-specific things into the generic > dispatcher. I believe it's easy enough to avoid; see below. > > > typedef struct QmpCommand > > { > > const char *name; > > @@ -35,7 +42,7 @@ typedef struct QmpCommand > > QmpCommandFunc *fn; > > QmpCommandOptions options; > > QTAILQ_ENTRY(QmpCommand) node; > > - bool enabled; > > + QmpDisabled disabled; > > } QmpCommand; > > > > typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList; > > @@ -44,7 +51,8 @@ void qmp_register_command(QmpCommandList *cmds, const > char *name, > > QmpCommandFunc *fn, QmpCommandOptions > options); > > const QmpCommand *qmp_find_command(const QmpCommandList *cmds, > > const char *name); > > -void qmp_disable_command(QmpCommandList *cmds, const char *name); > > +void qmp_disable_command(QmpCommandList *cmds, const char *name, > > + QmpDisabled disabled); > > void qmp_enable_command(QmpCommandList *cmds, const char *name); > > > > bool qmp_command_is_enabled(const QmpCommand *cmd); > > diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c > > index 509ae870bd..94a8e133b6 100644 > > --- a/monitor/qmp-cmds-control.c > > +++ b/monitor/qmp-cmds-control.c > > @@ -107,7 +107,7 @@ static void query_commands_cb(const QmpCommand *cmd, > void *opaque) > > CommandInfo *info; > > CommandInfoList **list = opaque; > > > > - if (!cmd->enabled) { > > + if (!qmp_command_is_enabled(cmd)) { > > return; > > } > > > > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > > index 0a2b20a4e4..b65f670152 100644 > > --- a/qapi/qmp-dispatch.c > > +++ b/qapi/qmp-dispatch.c > > @@ -155,11 +155,19 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, > QObject *request, > > "The command %s has not been found", command); > > goto out; > > } > > - if (!cmd->enabled) { > > + switch (cmd->disabled) { > > + case QMP_DISABLED_FROZEN: > > + error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND, > > + "The command %s has been disabled after fsfreeze", > > + command); > > + goto out; > > + case QMP_DISABLED_GENERIC: > > error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND, > > "The command %s has been disabled for this instance", > > command); > > goto out; > > + case QMP_DISABLED_NONE: > > + break; > > } > > if (oob && !(cmd->options & QCO_ALLOW_OOB)) { > > v1 put an optional error message template into struct QmpCommand, and > set the error with > > error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND, > cmd->err_msg ?: "The command %s has been disabled for > this instance", > command); > > Peter Krempa pointed out that this defeats the compiler's format string > checking. > > I feel the proper way to avoid this is to keep an optional string in > QmpCommand that explains the disablement, and use it like this: > > if (!cmd->enabled) { > error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND, > "Command %s has been disabled%s%s", > command, > cmd->disable_reason ? ": ", "", > cmd->disable_reason ?: ""); > goto out; > } > > Works for me, I'll send a v3. thanks If we make the string mandatory, we can ditch cmd->enabled, and say > > if (cmd->disabled) { > error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND, > "Command %s has been disabled: %s", > command, cmd->disabled); > goto out; > } > > [...] > >
