Re: [Qemu-devel] [PATCH v2 04/12] qemu-option: improve qemu_opts_print_help() output

[Max Reitz]

First of all, this patch broke iotest 082.  But then again, all that'd
be needed is a correction of the reference output.

However:

On 07.09.18 09:59, Marc-André Lureau wrote:
> Modify qemu_opts_print_help():
> - to print expected argument type
> - skip description if not available
> - sort lines
> - prefix with the list name (like qdev, to avoid confusion)

Is this really useful?  My main issue right now is this:

$ qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
qcow2-create-opts.backing_file=str - File name of a base image
qcow2-create-opts.backing_fmt=str - Image format of the base image
qcow2-create-opts.cluster_size=size - qcow2 cluster size
[...]

(The reason create is not affected is because it copies the options into
a new list.)

This is supposed to be a human-readable output.  I don't see how the
rather internal list name[1] really helps here.

([1] I suppose if you write a configuration file, these identifiers
become visible.  But you don't use "help" in a configuration file, so I
find that point becomes rather moot.)

So this is why I don't think it is a good idea to print this name.

Now if it helped to prevent confusion, OK, but without further
explanation I don't see how.  Is this because I can use "help" in places
where it's unclear to the user what exactly the "help" refers to?

Also, this is bikeshedding, but still, I don't like the dot notation
here.  It doesn't mean anything (I can't use
"qcow2-create-opts.backing_file", nor can I use
"virtio-blk-pci.iothread"), so I'd prefer a nice "human" prefix like
"(qcow2-create-opts) backing_file" or "qcow2-create-opts: ".

Actually, I don't like the whole notation.  It looks like code.  Here is
an excerpt from -device virtio-blk-pci,help:

virtio-blk-pci.iothread=link<iothread>
virtio-blk-pci.request-merging=bool (on/off)
virtio-blk-pci.logical_block_size=uint16 (A power of two between 512 and
32768)

I can imagine some ways this would be more readable to human users (and
I don't think ease of parsing is a high priority here), for instance:

virtio-blk-pci options:
iothread: link to object of type iothread
request-merging: bool (on/off)
logical_block_size: uint16 (A power of two between 512 and 32768)


(But it appears that transforming "link<iothread>" to something readable
wouldn't be easy.  Though then again, I really think it's unreadable
unless you know what it's supposed to mean.)


So my concrete requests are this:

1. If the ID is really useful, I would put it above the whole list in an
own line.  It's not useful to human readers to re-print it on every
single line.  It would be nice to have an option to disable this line,
however, if the caller has already printed something along the lines
(which qemu-img amend does).

2. Put some more whitespace into the whole thing, and make it less
robot-y overall.  I much prefer ": " when reading over "=" (even
programming languages do when it's about types, in fact).


I'm not writing a patch yet because maybe there is some reason to print
the ID on every single line.  So I'm just proposing first.

Max

> - drop 16-chars alignment, use a '-' as seperator for option name and
>   description
> 
> For ex, "-spice help" output is changed from:
> 
> port             No description available
> tls-port         No description available
> addr             No description available
> [...]
> gl               No description available
> rendernode       No description available
> 
> to:
> 
> spice.addr=str
> spice.agent-mouse=bool (on/off)
> spice.disable-agent-file-xfer=bool (on/off)
> [...]
> spice.x509-key-password=str
> spice.zlib-glz-wan-compression=str
> 
> "qemu-img create -f qcow2 -o help", changed from:
> 
> size             Virtual disk size
> compat           Compatibility level (0.10 or 1.1)
> backing_file     File name of a base image
> [...]
> lazy_refcounts   Postpone refcount updates
> refcount_bits    Width of a reference count entry in bits
> 
> to:
> 
> backing_file=str - File name of a base image
> backing_fmt=str - Image format of the base image
> cluster_size=size - qcow2 cluster size
> [...]
> refcount_bits=num - Width of a reference count entry in bits
> size=size - Virtual disk size
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  util/qemu-option.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 557b6c6626..069de36d2c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -208,17 +208,51 @@ out:
>      return result;
>  }
>  
> +static const char *opt_type_to_string(enum QemuOptType type)
> +{
> +    switch (type) {
> +    case QEMU_OPT_STRING:
> +        return "str";
> +    case QEMU_OPT_BOOL:
> +        return "bool (on/off)";
> +    case QEMU_OPT_NUMBER:
> +        return "num";
> +    case QEMU_OPT_SIZE:
> +        return "size";
> +    }
> +
> +    g_assert_not_reached();
> +}
> +
>  void qemu_opts_print_help(QemuOptsList *list)
>  {
>      QemuOptDesc *desc;
> +    int i;
> +    GPtrArray *array = g_ptr_array_new();
>  
>      assert(list);
>      desc = list->desc;
>      while (desc && desc->name) {
> -        printf("%-16s %s\n", desc->name,
> -               desc->help ? desc->help : "No description available");
> +        GString *str = g_string_new(NULL);
> +        if (list->name) {
> +            g_string_append_printf(str, "%s.", list->name);
> +        }
> +        g_string_append_printf(str, "%s=%s", desc->name,
> +                               opt_type_to_string(desc->type));
> +        if (desc->help) {
> +            g_string_append_printf(str, " - %s", desc->help);
> +        }
> +        g_ptr_array_add(array, g_string_free(str, false));
>          desc++;
>      }
> +
> +    g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp);
> +    for (i = 0; i < array->len; i++) {
> +        printf("%s\n", (char *)array->pdata[i]);
> +    }
> +    g_ptr_array_set_free_func(array, g_free);
> +    g_ptr_array_free(array, true);
> +
>  }
>  /* ------------------------------------------------------------------ */
>  
>

signature.asc
Description: OpenPGP digital signature