Re: [Qemu-devel] [PATCH v3] qemu-img.c: add help for each command

[Eric Blake]

On 9/25/18 10:39 AM, John Arbuckle wrote:
> Add the ability for the user to display help for a certain command.
> Example: qemu-img create --help
>
> What is printed is all the options available to this command and an example.
>
> Signed-off-by: John Arbuckle <address@hidden>
> ---
> v3 changes:
> Fixed a bug that caused qemu-img to crash when running a command without
> options.

Hmm, I just reviewed v2 without noticing that you had posted v3.

> v2 changes:
> Removed block of string comparison code for each command.
> Added a help_func field to the img_cmd_t struct.
> Made strings longer than 80 characters wide.
>
>   qemu-img.c | 660 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 559 insertions(+), 101 deletions(-)

My v2 comment about splitting this into more reviewable portions still 
applies.

> +++ b/qemu-img.c
> @@ -52,6 +52,7 @@
>   typedef struct img_cmd_t {
>       const char *name;
>       int (*handler)(int argc, char **argv);
> +    void (*help_func)(void);
>   } img_cmd_t;

As does my complaint that you want to track the command synopses here in 
this struct for reuse in multiple contexts later, rather than...

> +static void help_amend(void)
> +{
> +    const char *msg =
> +    "\n"
> +    "usage: qemu-img amend [--object objectdef] [--image-opts] [-p] [-q] [-f 
> fmt]\n"
> +    "[-t cache] -o options filename\n"

repeating things that are in qemu-img-cmds.h and risking them getting 
out of sync.

>   
> -static const img_cmd_t img_cmds[] = {
> +static img_cmd_t img_cmds[] = {
>   #define DEF(option, callback, arg_string)        \
>       { option, callback },
>   #include "qemu-img-cmds.h"
> @@ -4894,6 +5334,12 @@ static const img_cmd_t img_cmds[] = {
>       { NULL, NULL, },
>   };
>   
> +/* These functions will be added to the img_cmds array */
> +void (*help_functions[])(void) = {help_amend, help_bench, help_check,\
> +    help_commit, help_compare, help_convert, help_create, help_dd, help_info,\
> +    help_map, help_measure, help_snapshot, help_rebase, help_resize};

And my biggest gripe, that this approach to initialization is dead 
wrong, is still applicable.

Overall, I like the end result that you are trying to reach. (I'm not 
always a fan of having to run 'command --help' followed by 'command 
subcommand --help' to learn everything, but it sure beats a wall of 
text, and we have at least 'git' and 'cvs' as examples of programs that 
have used that approach, so we are not inventing something new).  But 
you'll need a v4, and preferably broken up into something easier to 
review (one patch per command, rather than a wholesale rewrite in go), 
if you want to get it in.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org