Re: [PATCH v2] qga: return a more explicit error on why a command is disabled

[Markus Armbruster]

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;
    }

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;
    }

[...]