monitor: print help for command errors

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

Quoted 6 lines · 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.

Quoted 13 lines

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)

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 <[email protected]>

---
 monitor.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

monitor.c +1 -1

index b2561e1..46e8880 100644
@@ -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;

monitor.c +5 -3

 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:

void monitor_set_error(Monitor *mon, QError *qerror)

monitor.c +1 -0

@@ -4114,20 +4118,22 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
     const mon_cmd_t *cmd;
+    int failed = 0;

Bandan Das <[email protected]> writes:

Quoted 8 lines

monitor.c +1 -1

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

monitor.c static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, · line 960

Quoted 5 lines

monitor.c +2 -0

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

monitor.c static const mon_cmd_t *monitor_parse_command(Monitor *mon, · line 4093

Quoted 8 lines

monitor.c +2 -1

 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.

Quoted 9 lines

void monitor_set_error(Monitor *mon, QError *qerror)

monitor.c +1 -0

@@ -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();
 

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4129

Quoted 7 lines

monitor.c +4 -2

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

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4132

Quoted 4 lines

monitor.c +1 -1

     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.

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4133

Quoted 1 line

monitor.c +0 -0

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

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4139

Quoted 8 lines

monitor.c +1 -2

         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.

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4147

Quoted 5 lines

monitor.c +1 -0

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

Recommend (cmd && failed).

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4152

Quoted 5 lines

monitor.c +3 -0

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

Quoted 2 lines · Markus Armbruster

Bandan Das <[email protected]> writes:

...

Quoted 8 lines

Quoted 8 lines

monitor.c +1 -1

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

Quoted 8 lines

monitor.c static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, · line 960

Quoted 5 lines

monitor.c +2 -0

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

Quoted 49 lines

monitor.c static const mon_cmd_t *monitor_parse_command(Monitor *mon, · line 4093

Quoted 8 lines

monitor.c +2 -1

 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
    }

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

void monitor_set_error(Monitor *mon, QError *qerror)

monitor.c +1 -0

@@ -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();
 

Quoted 10 lines

monitor.c static void handle_user_command(Monitor *mon, const char *cmdline) · line 4129

Quoted 7 lines

monitor.c +4 -2

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