marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@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.lureau@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;
}
[...]