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

[Bandan Das <bsd@redhat.com>]

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)