Re: [Qemu-devel] [PATCH] qmp: Give saner messages related to qmp_capabilities misuse

[Paulo Ricardo Paz Vital <paulo.vital@profitbricks.com>]

Reviewed-by: Paulo Vital <paulo.vital@profitbricks.com>

On Wed, 2015-04-15 at 09:19 -0600, Eric Blake wrote:
> Pretending that QMP doesn't understand a command merely because
> we are not in the right mode doesn't help first-time users figure
> out what to do to correct things.  Although the documentation for
> QMP calls out capabilities negotiation, we should also make it
> clear in our error messages what we were expecting.  With this
> patch, I now get the following transcript:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -nodefaults
> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}
> {"execute":"huh"}
> {"error": {"class": "CommandNotFound", "desc": "The command huh has not been found"}}
> {"execute":"quit"}
> {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities negotiation with 'qmp_capabilities' before command 'quit'"}}
> {"execute":"qmp_capabilities"}
> {"return": {}}
> {"execute":"qmp_capabilities"}
> {"error": {"class": "CommandNotFound", "desc": "Capabilities negotiation is already complete, command 'qmp_capabilities' ignored"}}
> {"execute":"quit"}
> {"return": {}}
> {"timestamp": {"seconds": 1429110729, "microseconds": 181935}, "event": "SHUTDOWN"}
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Based in part on this thread:
> https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00831.html
> 
>  monitor.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 68873ec..9f37700 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4783,10 +4783,22 @@ static int monitor_can_read(void *opaque)
>      return (mon->suspend_cnt == 0) ? 1 : 0;
>  }
> 
> -static int invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
> +static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
>  {
> -    int is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
> -    return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
> +    bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
> +    if (is_cap && qmp_cmd_mode(mon)) {
> +        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
> +                      "Capabilities negotiation is already complete, command "
> +                      "'%s' ignored", cmd->name);
> +        return true;
> +    }
> +    if (!is_cap && !qmp_cmd_mode(mon)) {
> +        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
> +                      "Expecting capabilities negotiation with "
> +                      "'qmp_capabilities' before command '%s'", cmd->name);
> +        return true;
> +    }
> +    return false;
>  }
> 
>  /*
> @@ -5080,11 +5092,14 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>      cmd_name = qdict_get_str(input, "execute");
>      trace_handle_qmp_command(mon, cmd_name);
>      cmd = qmp_find_cmd(cmd_name);
> -    if (!cmd || invalid_qmp_mode(mon, cmd)) {
> +    if (!cmd) {
>          qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
>                        "The command %s has not been found", cmd_name);
>          goto err_out;
>      }
> +    if (invalid_qmp_mode(mon, cmd)) {
> +        goto err_out;
> +    }
> 
>      obj = qdict_get(input, "arguments");
>      if (!obj) {