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

[Marc-André Lureau]

Hi

On Tue, Oct 9, 2018 at 2:03 AM Max Reitz <address@hidden> wrote:
>
> First of all, this patch broke iotest 082.  But then again, all that'd
> be needed is a correction of the reference output.

Oops, ok. Let see if we change it again,

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

I didn't realize this would change qemu-img help in such a way (I
checked create output, my bad...).

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

I think that's one of the reason why the original qdev help did prefix
with the class/driver name. Although in practice, it exit() after, so
I am not sure it's really helpeful (unless there is some help that can
be printed before?).

Another reason to want the prefix is to be close to the -global
syntax. Again, probably not worthwhile.


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

That would be fine for me.

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

I think we have stronger reasons to want it remove now.

Feel free to send a patch

thanks

>
> 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);
> > +
> >  }
> >  /* ------------------------------------------------------------------ */
> >
> >
>
>


-- 
Marc-André Lureau