[Qemu-devel] [PATCH] monitor: print help for command errors

[Qemu-devel] [PATCH] monitor: print help for command errors

[Bandan Das]

Unlike machines, humans will be (mostly) appreciative on seeing
help output when a command fails due to incorrect syntax or input.
By default, print output of help_cmd() to the monitor in such cases.
The only exceptions are if a command does not exist or parsing
failed for some other reason.

Before:
(qemu) drive_add usb_flash_drive
drive_add: string expected
After:
(qemu) drive_add usb_flash_drive
drive_add: string expected
Usage:
drive_add [[<domain>:]<bus>:]<slot>
[file=file][,if=type][,bus=n]
[,unit=m][,media=d][,index=i]
[,cyls=c,heads=h,secs=s[,trans=t]]
[,snapshot=on|off][,cache=on|off]
[,readonly=on|off][,copy-on-read=on|off] -- add drive to PCI storage controller

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..37f00d9 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;
 }
 
 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;
 }
 
 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;
+    }
 
     if (handler_is_async(cmd)) {
-        user_async_cmd_handler(mon, cmd, qdict);
+        failed = user_async_cmd_handler(mon, cmd, qdict);
     } else if (handler_is_qobject(cmd)) {
         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);
@@ -4138,6 +4144,10 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
     }
 
 out:
+    if (failed && cmd) {
+        monitor_printf(mon, "Usage:\n");
+        help_cmd(mon, cmd->name);
+    }
     QDECREF(qdict);
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH] monitor: print help for command errors

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> Unlike machines, humans will be (mostly) appreciative on seeing
> help output when a command fails due to incorrect syntax or input.
> By default, print output of help_cmd() to the monitor in such cases.
> The only exceptions are if a command does not exist or parsing
> failed for some other reason.
>
> Before:
> (qemu) drive_add usb_flash_drive
> drive_add: string expected
> After:
> (qemu) drive_add usb_flash_drive
> drive_add: string expected
> Usage:
> drive_add [[<domain>:]<bus>:]<slot>
> [file=file][,if=type][,bus=n]
> [,unit=m][,media=d][,index=i]
> [,cyls=c,heads=h,secs=s[,trans=t]]
> [,snapshot=on|off][,cache=on|off]
> [,readonly=on|off][,copy-on-read=on|off] -- add drive to PCI storage controller

I'm sure users will appreciate a little help on error.  What I don't
appreciate is having to search lengthy output for the error message :)

Our help consists of two parts:

1. .params, of the form: COMMAND-NAME PARAMETER-NAME...
2. .help, which is a free-form HELP-TEXT

Following the help command's example, you print them run together like

    COMMAND-NAME PARAMETER-NAME... -- HELP-TEXT

which can easily lead to output lines that are too long for easy
reading, e.g.

    hostfwd_add [vlan_id name] [tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport -- redirect TCP or UDP connections from host to guest (requires -net user)

Worse, a few commands have multi-line HELP-TEXT:

    block_job_cancel
    drive_backup
    drive_mirror
    dump-guest-memory
    migrate
    migrate_set_cache_size
    pcie_aer_inject_error
    snapshot_blkdev
    snapshot_blkdev_internal
    snapshot_delete_blkdev_internal

One even has multi-line PARAMETER-NAME..: drive_add.  That produces the
ugliest help of all; thanks for picking it as your example.

Two ways to avoid burying the error message in an avalanche of help:

A. Instead of possibly lengthy help, say how to get it

        (qemu) drive_add usb_flash_drive
        drive_add: string expected
        Try "help drive_add" for more information.

   For what it's worth, it's what many shell utilities do.

B. Ensure the help is brief, and easy to read

   Don't run together PARAMETER-NAME and HELP-TEXT, put them on separate
   lines.

   Print only the first line of HELP-TEXT.  Check the HELP-TEXTs have a
   sensible first line.

   When this doesn't print the full HELP-TEXT (because it has more than
   one line), add how to get complete help.

        (qemu) drive_backup 
        drive_backup: block device name expected
        Usage: drive_backup [-n] [-f] device target [format]
        initiates a point-in-time copy for a device
        Try "help drive_backup" for more information.

I prefer A.

Speaking of help, output of "help" is awful.  It's a flood of badly
formatted text, with long lines and weird indentation.  I'd rather see
one line per command, then a final line explaining how to get more help
on a specific command.  Something like

        (qemu) help
        List of commands:
        acl_add -- Add a match rule to the access control list
        [...]
        drive_backup -- Initiate a point-in-time copy for a device
        [...]
        xp -- Physical memory dump starting at 'addr'
        Try "help" followed by a command name for full command help.
        (qemu) help drive_backup
        drive_backup [-n] [-f] device target [format]
        Initiate a point-in-time copy for a device
        The device's contents are copied to the new image file,
        excluding data that is written after the command is started.
        The -n flag requests QEMU to reuse the image found in
        new-image-file, instead of recreating it from scratch.  The -f
        flag requests QEMU to copy the whole disk, so that the result
        does not need a backing file.
        (qemu)

Better, but "help" still floods the terminal with about 100 commands.
gdb avoids this by grouping commands:

        (gdb) help
        List of classes of commands:

        aliases -- Aliases of other commands
        breakpoints -- Making program stop at certain points
        data -- Examining data
        files -- Specifying and examining files
        internals -- Maintenance commands
        obscure -- Obscure features
        running -- Running the program
        stack -- Examining the stack
        status -- Status inquiries
        support -- Support facilities
        tracepoints -- Tracing of program execution without stopping the program
        user-defined -- User-defined commands

        Type "help" followed by a class name for a list of commands in that class.
        Type "help all" for the list of all commands.
        Type "help" followed by command name for full documentation.
        Type "apropos word" to search for commands related to "word".
        Command name abbreviations are allowed if unambiguous.
        (gdb) help stack
        Examining the stack.
        The stack is made up of stack frames.  Gdb assigns numbers to stack frames
        counting from zero for the innermost (currently executing) frame.

        At any time gdb identifies one frame as the "selected" frame.
        Variable lookups are done with respect to the selected frame.
        When the program being debugged stops, gdb selects the innermost frame.
        The commands below can be used to select other frames by number or address.

        List of commands:

        backtrace -- Print backtrace of all stack frames
        bt -- Print backtrace of all stack frames
        down -- Select and print stack frame called by this one
        frame -- Select and print a stack frame
        return -- Make selected stack frame return to its caller
        select-frame -- Select a stack frame without printing anything
        up -- Select and print stack frame that called this one

        Type "help" followed by command name for full documentation.
        Type "apropos word" to search for commands related to "word".
        Command name abbreviations are allowed if unambiguous.
        (gdb) help bt
        Print backtrace of all stack frames, or innermost COUNT frames.
        With a negative argument, print outermost -COUNT frames.
        Use of the 'full' qualifier also prints the values of the local variables.
        Use of the 'no-filters' qualifier prohibits frame filters from executing
        on this backtrace.

        (gdb) 

Re: [Qemu-devel] [PATCH] monitor: print help for command errors

[Bandan Das]

Markus Armbruster <armbru@redhat.com> writes:

> Bandan Das <bsd@redhat.com> writes:
>
>> Unlike machines, humans will be (mostly) appreciative on seeing
>> help output when a command fails due to incorrect syntax or input.
>> By default, print output of help_cmd() to the monitor in such cases.
>> The only exceptions are if a command does not exist or parsing
>> failed for some other reason.
>>
>> Before:
>> (qemu) drive_add usb_flash_drive
>> drive_add: string expected
>> After:
>> (qemu) drive_add usb_flash_drive
>> drive_add: string expected
>> Usage:
>> drive_add [[<domain>:]<bus>:]<slot>
>> [file=file][,if=type][,bus=n]
>> [,unit=m][,media=d][,index=i]
>> [,cyls=c,heads=h,secs=s[,trans=t]]
>> [,snapshot=on|off][,cache=on|off]
>> [,readonly=on|off][,copy-on-read=on|off] -- add drive to PCI storage controller
>
> I'm sure users will appreciate a little help on error.  What I don't
> appreciate is having to search lengthy output for the error message :)

I don't think that's true. The error message is still unchanged and is
*always* the first line. That it's completely useless in most cases
is a different matter though.

> Our help consists of two parts:
>
> 1. .params, of the form: COMMAND-NAME PARAMETER-NAME...
> 2. .help, which is a free-form HELP-TEXT
>
> Following the help command's example, you print them run together like
>
>     COMMAND-NAME PARAMETER-NAME... -- HELP-TEXT
>
> which can easily lead to output lines that are too long for easy
> reading, e.g.
>
>     hostfwd_add [vlan_id name] [tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport -- redirect TCP or UDP connections from host to guest (requires -net user)
>
> Worse, a few commands have multi-line HELP-TEXT:
>
>     block_job_cancel
>     drive_backup
>     drive_mirror
>     dump-guest-memory
>     migrate
>     migrate_set_cache_size
>     pcie_aer_inject_error
>     snapshot_blkdev
>     snapshot_blkdev_internal
>     snapshot_delete_blkdev_internal
>
> One even has multi-line PARAMETER-NAME..: drive_add.  That produces the
> ugliest help of all; thanks for picking it as your example.

Ugly yes, but still helpful! I guess when a user gets it wrong,
inevitably she ends up typing "help cmdname" anyway. However, the very
thought of me getting a command syntax wrong and the buffer automatically
scrolling more than two pagefuls is already making me flinch ;)

> Two ways to avoid burying the error message in an avalanche of help:
>
> A. Instead of possibly lengthy help, say how to get it
>
>         (qemu) drive_add usb_flash_drive
>         drive_add: string expected
>         Try "help drive_add" for more information.
>
>    For what it's worth, it's what many shell utilities do.

Right. Agreed, It's better to just follow conventions in these
cases. I will send a v2 with this change.

> B. Ensure the help is brief, and easy to read
>
>    Don't run together PARAMETER-NAME and HELP-TEXT, put them on separate
>    lines.
>
>    Print only the first line of HELP-TEXT.  Check the HELP-TEXTs have a
>    sensible first line.
>
>    When this doesn't print the full HELP-TEXT (because it has more than
>    one line), add how to get complete help.
>
>         (qemu) drive_backup 
>         drive_backup: block device name expected
>         Usage: drive_backup [-n] [-f] device target [format]
>         initiates a point-in-time copy for a device
>         Try "help drive_backup" for more information.
>
> I prefer A.
>
> Speaking of help, output of "help" is awful.  It's a flood of badly
> formatted text, with long lines and weird indentation.  I'd rather see
> one line per command, then a final line explaining how to get more help
> on a specific command.  Something like
>
>         (qemu) help
>         List of commands:
>         acl_add -- Add a match rule to the access control list
>         [...]
>         drive_backup -- Initiate a point-in-time copy for a device
>         [...]
>         xp -- Physical memory dump starting at 'addr'
>         Try "help" followed by a command name for full command help.
>         (qemu) help drive_backup
>         drive_backup [-n] [-f] device target [format]
>         Initiate a point-in-time copy for a device
>         The device's contents are copied to the new image file,
>         excluding data that is written after the command is started.
>         The -n flag requests QEMU to reuse the image found in
>         new-image-file, instead of recreating it from scratch.  The -f
>         flag requests QEMU to copy the whole disk, so that the result
>         does not need a backing file.
>         (qemu)
>
> Better, but "help" still floods the terminal with about 100 commands.
> gdb avoids this by grouping commands:

Good ideas. I imagine it's going to be a complete rewrite :) When I have
some free cycles, I will think a little more about this.

Thank you for the thorough review.
Bandan

>         (gdb) help
>         List of classes of commands:
>
>         aliases -- Aliases of other commands
>         breakpoints -- Making program stop at certain points
>         data -- Examining data
>         files -- Specifying and examining files
>         internals -- Maintenance commands
>         obscure -- Obscure features
>         running -- Running the program
>         stack -- Examining the stack
>         status -- Status inquiries
>         support -- Support facilities
>         tracepoints -- Tracing of program execution without stopping the program
>         user-defined -- User-defined commands
>
>         Type "help" followed by a class name for a list of commands in that class.
>         Type "help all" for the list of all commands.
>         Type "help" followed by command name for full documentation.
>         Type "apropos word" to search for commands related to "word".
>         Command name abbreviations are allowed if unambiguous.
>         (gdb) help stack
>         Examining the stack.
>         The stack is made up of stack frames.  Gdb assigns numbers to stack frames
>         counting from zero for the innermost (currently executing) frame.
>
>         At any time gdb identifies one frame as the "selected" frame.
>         Variable lookups are done with respect to the selected frame.
>         When the program being debugged stops, gdb selects the innermost frame.
>         The commands below can be used to select other frames by number or address.
>
>         List of commands:
>
>         backtrace -- Print backtrace of all stack frames
>         bt -- Print backtrace of all stack frames
>         down -- Select and print stack frame called by this one
>         frame -- Select and print a stack frame
>         return -- Make selected stack frame return to its caller
>         select-frame -- Select a stack frame without printing anything
>         up -- Select and print stack frame that called this one
>
>         Type "help" followed by command name for full documentation.
>         Type "apropos word" to search for commands related to "word".
>         Command name abbreviations are allowed if unambiguous.
>         (gdb) help bt
>         Print backtrace of all stack frames, or innermost COUNT frames.
>         With a negative argument, print outermost -COUNT frames.
>         Use of the 'full' qualifier also prints the values of the local variables.
>         Use of the 'no-filters' qualifier prohibits frame filters from executing
>         on this backtrace.
>
>         (gdb) 

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

[Bandan Das]

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;
 }
 
 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;
 }
 
 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;
+    }
 
     if (handler_is_async(cmd)) {
-        user_async_cmd_handler(mon, cmd, qdict);
+        failed = user_async_cmd_handler(mon, cmd, qdict);
     } else if (handler_is_qobject(cmd)) {
         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);
@@ -4138,6 +4144,10 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
     }
 
 out:
+    if (failed && cmd) {
+        monitor_printf(mon, "Try \"help %s\" for more information\n",
+                       cmd->name);
+    }
     QDECREF(qdict);
 }
 
-- 
2.1.0

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

[Markus Armbruster]

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.

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

[Bandan Das]

Markus Armbruster <armbru@redhat.com> writes:

> Bandan Das <bsd@redhat.com> writes:
>
...
>> -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.

Ok or if you prefer, I can skip this part and other similar cases below.
...
>>  
>>  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.

Ok understood, makes sense. Will fix this in next version.

>>  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)?

Note that I changed the old behavior so that now monitor_parse_command()
returns cmd when failed is set. This is so that we have cmd->name. "if (!cmd)"
will be false in that case.

>>  
>>      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.

Ok.

>>          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).

Since this path is common whether the command failed or not, I
think it's better to check for "failed" as the first condition.
So, the check on second argument need not be done if the function
succeeds. Actually, taking a second look, I think just
"if (failed) {" should be enough since cmd is guaranteed to be !NULL
when failed is set.

Bandan

>> +        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.

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

[Markus Armbruster]

Bandan Das <bsd@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Bandan Das <bsd@redhat.com> writes:
>>
> ...
>>> -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.
>
> Ok or if you prefer, I can skip this part and other similar cases below.
> ...

I think limiting the additional help to argument syntax errors for now
will be easiest.

>>>  
>>>  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.
>
> Ok understood, makes sense. Will fix this in next version.

Looking forward to it :)

>>>  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)?
>
> Note that I changed the old behavior so that now monitor_parse_command()
> returns cmd when failed is set. This is so that we have cmd->name. "if (!cmd)"
> will be false in that case.

You're right.  I got confused, figured out what's up, updated my review,
but missed this paragraph.

[...]