| Message ID | 20180906151227.25514-11-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Various qemu command line options help improvements | expand |
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. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > 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? > + 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.
Hi On Thu, Sep 6, 2018 at 7:40 PM Eric Blake <eblake@redhat.com> 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 <marcandre.lureau@redhat.com> > > --- > > 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 >
On 09/06/2018 11:27 AM, Marc-André Lureau wrote: > 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. I don't have a strong preference for one form over the other, so much as consistency in the various applications using the form. >>> +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? Does glib not already have such a helper? cutils sounds as good a place as any, although inline may be at odds with typically using it as a callback function (I don't know how well the compiler and linker handle such a situation). > >> >> >>> + 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... I don't worry too much about that. We've already fixed other places where qemu binaries were antisocial, such as commit ac1307ab fixing 'qemu-img --help' to give status 0 instead of 1. In general, when a user asks for help, the help text should go to stdout, and the exit status should be 0 (unless you go to great lengths to also detect write failures such as ENOSPC or EPIPE, in which case you should ALSO attempt to write to stderr prior to exiting with nonzero status that you couldn't output the help text - but since FILE* I/O can cache things, detecting all possible write errors is not reliable unless you check the result of fclose(stdout), which in turn has to be deferred to the end of your program execution, generally via atexit(). The extra complications and code maintenance for checking for --help output failures is something that I'm personally okay with skipping).
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index 72b97a8bed..941fd63afd 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -141,14 +141,14 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp) int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp) { - bool (*type_predicate)(const char *) = opaque; + bool (*type_opt_predicate)(const char *, QemuOpts *) = opaque; Object *obj = NULL; Error *err = NULL; const char *type; type = qemu_opt_get(opts, "qom-type"); - if (type && type_predicate && - !type_predicate(type)) { + if (type && type_opt_predicate && + !type_opt_predicate(type, opts)) { return 0; } diff --git a/vl.c b/vl.c index 8a5fd0c81f..75c2bd5130 100644 --- a/vl.c +++ 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); +} /* * Initial object creation happens before all other @@ -2729,8 +2734,10 @@ static int machine_set_property(void *opaque, * cannot be created here, as it depends on the chardev * already existing. */ -static bool object_create_initial(const char *type) +static bool object_create_initial(const char *type, QemuOpts *opts) { + ObjectClass *klass; + if (is_help_option(type)) { GSList *l, *list; @@ -2744,6 +2751,38 @@ static bool object_create_initial(const char *type) exit(0); } + klass = object_class_by_name(type); + if (klass && qemu_opt_has_help_opt(opts)) { + ObjectPropertyIterator iter; + ObjectProperty *prop; + GPtrArray *array = g_ptr_array_new(); + int i; + + object_class_property_iter_init(&iter, klass); + while ((prop = object_property_iter_next(&iter))) { + GString *str; + + if (!prop->set) { + continue; + } + + str = g_string_new(NULL); + g_string_append_printf(str, "%s.%s=%s", type, + prop->name, prop->type); + if (prop->description) { + g_string_append_printf(str, " (%s)", prop->description); + } + g_ptr_array_add(array, g_string_free(str, false)); + } + 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); + } + if (g_str_equal(type, "rng-egd") || g_str_has_prefix(type, "pr-manager-")) { return false; @@ -2790,9 +2829,9 @@ static bool object_create_initial(const char *type) * The remainder of object creation happens after the * creation of chardev, fsdev, net clients and device data types. */ -static bool object_create_delayed(const char *type) +static bool object_create_delayed(const char *type, QemuOpts *opts) { - return !object_create_initial(type); + return !object_create_initial(type, opts); }
Iterate over the writable class properties, sort and print them out with the description if available. Ex: qemu -object memory-backend-memfd,? 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)) Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- qom/object_interfaces.c | 6 +++--- vl.c | 45 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 6 deletions(-)