Re: [Qemu-devel] [PATCH 10/10] vl: list user creatable propeties if '?' as argument

[Marc-André Lureau]

Hi

On Thu, Sep 6, 2018 at 7:40 PM Eric Blake <address@hidden> wrote:
>
> On 09/06/2018 10:12 AM, Marc-André Lureau wrote:
>
> Subject has typo and awkward grammar; I'd suggest:
>
> vl: list user creatable properties when 'help' is argument
>
> > Iterate over the writable class properties, sort and print them out
> > with the description if available.
> >
> > Ex: qemu -object memory-backend-memfd,?
>
> As elsewhere in the series, s/\?/help/
>
> > memory-backend-memfd.dump=bool (Set to 'off' to exclude from core dump)
> > memory-backend-memfd.host-nodes=int (Binds memory to the list of NUMA host 
> > nodes)
> > memory-backend-memfd.hugetlb=bool (Use huge pages)
> > memory-backend-memfd.hugetlbsize=int (Huge pages size (ex: 2M, 1G))
> > memory-backend-memfd.merge=bool (Mark memory as mergeable)
> > memory-backend-memfd.policy=HostMemPolicy (Set the NUMA policy)
> > memory-backend-memfd.prealloc=bool (Preallocate memory)
> > memory-backend-memfd.seal=bool (Seal growing & shrinking)
> > memory-backend-memfd.share=bool (Mark the memory as private to QEMU or 
> > shared)
> > memory-backend-memfd.size=int (Size of the memory region (ex: 500M))
>
> Why "name=type (text)" here, but "name=type - text" in 2/10?
> Consistency would be nice.

We use name=type (text) for devices properties, ex:

qemu-system-x86_64 -device tpm-tis,?
tpm-tis.tpmdev=str (ID of a tpm to use as a backend)
tpm-tis.irq=uint32
tpm-tis.tpm-tis-mmio[0]=child<qemu:memory-region>

But
qemu-img create -f qcow2 -o ?

    size             Virtual disk size
    compat           Compatibility level (0.10 or 1.1)
    backing_file     File name of a base image

I think I like more "name=type - text" form I introduced in "improve
qemu_opts_print_help() output". I guess I should change device
properties help for consistency then.

> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >   qom/object_interfaces.c |  6 +++---
> >   vl.c                    | 45 ++++++++++++++++++++++++++++++++++++++---
> >   2 files changed, 45 insertions(+), 6 deletions(-)
> >
>
> > +++ b/vl.c
> > @@ -2721,6 +2721,11 @@ static int machine_set_property(void *opaque,
> >       return 0;
> >   }
> >
> > +static gint
> > +pstrcmp(const char **a, const char **b)
> > +{
> > +    return g_strcmp0(*a, *b);
> > +}
>
> This is the second time your series has added this static helper. Should
> it be a common helper instead?

as qemu_pstrcmp in cutils? inline in the header?

>
>
> > +        g_ptr_array_sort(array, (GCompareFunc)pstrcmp);
> > +        for (i = 0; i < array->len; i++) {
> > +            error_printf("%s\n", (char *)array->pdata[i]);
> > +        }
> > +        g_ptr_array_set_free_func(array, g_free);
> > +        g_ptr_array_free(array, true);
> > +        exit(0);
>
> Again, printing to stderr then exiting with status 0 is awkward.  Print
> to stdout when successfully offering help text.

We use error_printf() for qdev list (qdev_device_help), which
redirects to monitor or stderr.

Should I also change ir for consistency? hopefully nobody relies on
the output going to stderr...

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


-- 
Marc-André Lureau