Hi
On Tue, Oct 9, 2018 at 2:03 AM Max Reitz <mre...@redhat.com> 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 <marcandre.lur...@redhat.com>
> > Reviewed-by: Eric Blake <ebl...@redhat.com>
> > ---
> > 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
