Re: [Qemu-devel] [PATCH] monitor: suggest running "help" for command errors

[Markus Armbruster <armbru@redhat.com>]

Bandan Das <bsd@redhat.com> writes:

> When a command fails due to incorrect syntax or input,
> suggest using the "help" command to get more information
> about the command. This is only applicable for HMP.
>
> Before:
> (qemu) drive_add usb_flash_drive
> drive_add: string expected
> After:
> (qemu) drive_add usb_flash_drive
> drive_add: string expected
> Try "help drive_add" for more information
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  monitor.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b2561e1..46e8880 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -939,7 +939,7 @@ static int qmp_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>      return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
>  }
>  
> -static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
> +static int user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>                                     const QDict *params)
>  {
>      int ret;
> @@ -954,6 +954,8 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
>          monitor_resume(mon);
>          g_free(cb_data);
>      }
> +
> +    return ret;
>  }
>  

user_async_cmd_handler() is unreachable since commit 3b5704b.  I got
code cleaning that up in my tree.  Don't worry about it.

>  static void hmp_info_help(Monitor *mon, const QDict *qdict)
> @@ -3698,7 +3700,8 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                                                const char *cmdline,
>                                                int start,
>                                                mon_cmd_t *table,
> -                                              QDict *qdict)
> +                                              QDict *qdict,
> +                                              int *failed)
>  {
>      const char *p, *typestr;
>      int c;
> @@ -3734,7 +3737,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>              return cmd;
>          }
>          return monitor_parse_command(mon, cmdline, p - cmdline,
> -                                     cmd->sub_table, qdict);
> +                                     cmd->sub_table, qdict, failed);
>      }
>  
>      /* parse the parameters */
> @@ -4084,8 +4087,9 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>      return cmd;
>  
>  fail:
> +    *failed = 1;
>      g_free(key);
> -    return NULL;
> +    return cmd;
>  }
>  

>From the function's contract:

 * If @cmdline is blank, return NULL.
 * If it can't be parsed, report to @mon, and return NULL.
 * Else, insert command arguments into @qdict, and return the command.

Your patch splits the "it can't be parsed" case into "command not found"
and "arguments can't be parsed", but neglects to update the contract.
Needs fixing.  Perhaps like this:

 * If @cmdline is blank, return NULL.
 * If @cmdline doesn't start with a valid command name, report to @mon,
 * and return NULL.
 * Else, if the command's arguments can't be parsed, report to @mon,
 * store 1 through @failed, and return the command.
 * Else, insert command arguments into @qdict, and return the command.

The contract wasn't exactly pretty before, and I'm afraid it's
positively ugly now.

The common cause for such ugliness is doing too much in one function.
I'd try splitting into a command part and an argument part, so that

    cmd = monitor_parse_command(mon, cmdline, start, table, qdict);
    if (!cmd) {
        // bail out
    }

becomes something like

    cmd = monitor_parse_command(mon, cmdline, start, table, &args);
    if (!cmd) {
        // bail out
    }
    qdict = monitor_parse_arguments(mon, cmd, args)
    if (!qdict) {
        // bail out
    }

While I encourage you to do this, I won't reject your patch just because
you don't.

>  void monitor_set_error(Monitor *mon, QError *qerror)
> @@ -4114,20 +4118,22 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>  {
>      QDict *qdict;
>      const mon_cmd_t *cmd;
> +    int failed = 0;
>  
>      qdict = qdict_new();
>  
> -    cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict);
> -    if (!cmd)
> +    cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table,
> +                                qdict, &failed);
> +    if (!cmd || failed) {
>          goto out;
> +    }

cmd implies !failed, therefore !cmd || failed == !cmd here.  Why not
simply stick to if (!cmd)?

>  
>      if (handler_is_async(cmd)) {
> -        user_async_cmd_handler(mon, cmd, qdict);
> +        failed = user_async_cmd_handler(mon, cmd, qdict);

As discussed above: unreachable, but don't worry about it.

>      } else if (handler_is_qobject(cmd)) {

This is the "new" HMP command interface.  It's used only with drive_del,
device_add, client_migrate_info, pcie_aer_inject_error.  The cleanup
work I mentioned above will get rid of it.  Again, don't worry.

>          QObject *data = NULL;
>  
> -        /* XXX: ignores the error code */
> -        cmd->mhandler.cmd_new(mon, qdict, &data);
> +        failed = cmd->mhandler.cmd_new(mon, qdict, &data);
>          assert(!monitor_has_error(mon));
>          if (data) {
>              cmd->user_print(mon, data);
               qobject_decref(data);
           }
       } else {

This is the traditional HMP command interface.  Almost all commands use
it, and after my pending cleanup, it'll be the *only* HMP command
interface.

It lacks means to communicate command failure.

           cmd->mhandler.cmd(mon, qdict);
       }

Since you can't set failed in the common case, I recommend not to bother
setting it in the unreachable and the uncommon case, either.

> @@ -4138,6 +4144,10 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>      }
>  
>  out:
> +    if (failed && cmd) {

Recommend (cmd && failed).

> +        monitor_printf(mon, "Try \"help %s\" for more information\n",
> +                       cmd->name);
> +    }
>      QDECREF(qdict);
>  }

Cases:

1. @cmdline is blank

   cmd == NULL && !failed

   We don't print command help.  Good.

2. @cmdline isn't blank, command not found

   cmd == NULL && !failed

   We don't print command help.  We already printed the error.  Good.

3. command found, arguments can't be parsed

   cmd != NULL && failed

   We print command help.  We printed the parse error already.  Good.

4. command found, arguments parsed, command failed

   cmd != NULL, but failed can be anything

   We may or may not print command help.  The command should've printed
   an error already.  Best we can do as long as the traditional HMP
   command handler doesn't return status.

5. command found, arguments parsed, command succeeded

   We don't print command help.  Good.

Works.