[Qemu-devel] [PATCH 00/10] Various qemu command line options help improvements

[Qemu-devel] [PATCH 00/10] Various qemu command line options help improvements

[Marc-André Lureau]

Hi,

This is a compilation of patches I have to improve command line help
support. The first 2 patches have already been sent earlier, I
modified the first to fix an issue reported by Markus. The other
patches add support for -object help. A few preliminary patches for
QOM, to fix/improve some minor issues.

Marc-André Lureau (10):
  qemu-option: add help fallback to print the list of options
  qemu-option: improve qemu_opts_print_help() output
  qom/object: fix iterating properties over a class
  qom/object: register 'type' property as class property
  tests/qom-proplist: check duplicate "bv" property registration failed
  tests/qom-proplist: check properties are not listed multiple times
  tests/qom-proplist: check class properties iterator
  vl: handle -object ?
  hostmem: add some properties description
  vl: list user creatable propeties if '?' as argument

 backends/hostmem-memfd.c   |  9 +++++
 backends/hostmem.c         | 14 +++++++
 qom/object.c               |  9 +++--
 qom/object_interfaces.c    |  6 +--
 tests/check-qom-proplist.c | 58 +++++++++++++++++-----------
 util/qemu-option.c         | 77 +++++++++++++++++++++++++++++++-------
 vl.c                       | 58 ++++++++++++++++++++++++++--
 7 files changed, 187 insertions(+), 44 deletions(-)

-- 
2.19.0.rc1

[Qemu-devel] [PATCH 01/10] qemu-option: add help fallback to print the list of options

[Marc-André Lureau]

QDev options accept '?' or 'help' in the list of parameters, which is
really handy to list the available options.

Unfortunately, this isn't built in QemuOpts. qemu_opts_parse_noisily()
seems to be the common path for command line options, so place a
fallback to check for '?' and print help listing available options.

This is very handy, for example with qemu "-spice ?".

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/qemu-option.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 01886efe90..557b6c6626 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -486,7 +486,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
 }
 
 static void opt_set(QemuOpts *opts, const char *name, char *value,
-                    bool prepend, Error **errp)
+                    bool prepend, bool *invalidp, Error **errp)
 {
     QemuOpt *opt;
     const QemuOptDesc *desc;
@@ -496,6 +496,9 @@ static void opt_set(QemuOpts *opts, const char *name, char *value,
     if (!desc && !opts_accepts_any(opts)) {
         g_free(value);
         error_setg(errp, QERR_INVALID_PARAMETER, name);
+        if (invalidp) {
+            *invalidp = true;
+        }
         return;
     }
 
@@ -519,7 +522,7 @@ static void opt_set(QemuOpts *opts, const char *name, char *value,
 void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
                   Error **errp)
 {
-    opt_set(opts, name, g_strdup(value), false, errp);
+    opt_set(opts, name, g_strdup(value), false, NULL, errp);
 }
 
 void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
@@ -750,7 +753,8 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
 }
 
 static void opts_do_parse(QemuOpts *opts, const char *params,
-                          const char *firstname, bool prepend, Error **errp)
+                          const char *firstname, bool prepend,
+                          bool *invalidp, Error **errp)
 {
     char *option = NULL;
     char *value = NULL;
@@ -785,7 +789,7 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
         }
         if (strcmp(option, "id") != 0) {
             /* store and parse */
-            opt_set(opts, option, value, prepend, &local_err);
+            opt_set(opts, option, value, prepend, invalidp, &local_err);
             value = NULL;
             if (local_err) {
                 error_propagate(errp, local_err);
@@ -814,11 +818,12 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
 void qemu_opts_do_parse(QemuOpts *opts, const char *params,
                        const char *firstname, Error **errp)
 {
-    opts_do_parse(opts, params, firstname, false, errp);
+    opts_do_parse(opts, params, firstname, false, NULL, errp);
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
-                            bool permit_abbrev, bool defaults, Error **errp)
+                            bool permit_abbrev, bool defaults,
+                            bool *invalidp, Error **errp)
 {
     const char *firstname;
     char *id = NULL;
@@ -850,7 +855,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         return NULL;
     }
 
-    opts_do_parse(opts, params, firstname, defaults, &local_err);
+    opts_do_parse(opts, params, firstname, defaults, invalidp, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         qemu_opts_del(opts);
@@ -870,7 +875,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
                           bool permit_abbrev, Error **errp)
 {
-    return opts_parse(list, params, permit_abbrev, false, errp);
+    return opts_parse(list, params, permit_abbrev, false, NULL, errp);
 }
 
 /**
@@ -886,10 +891,16 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
 {
     Error *err = NULL;
     QemuOpts *opts;
+    bool invalidp = false;
 
-    opts = opts_parse(list, params, permit_abbrev, false, &err);
+    opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
     if (err) {
-        error_report_err(err);
+        if (invalidp && has_help_option(params)) {
+            qemu_opts_print_help(list);
+            error_free(err);
+        } else {
+            error_report_err(err);
+        }
     }
     return opts;
 }
@@ -899,7 +910,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
 {
     QemuOpts *opts;
 
-    opts = opts_parse(list, params, permit_abbrev, true, NULL);
+    opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL);
     assert(opts);
 }
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 02/10] qemu-option: improve qemu_opts_print_help() output

[Marc-André Lureau]

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)
- drop 16-chars alignment, use a '-' as seperator for option name and
  description

For ex, "-spice ?" 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 ?", 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.lureau@redhat.com>
---
 util/qemu-option.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 557b6c6626..a54cf74b49 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -208,17 +208,57 @@ 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();
+}
+
+static gint
+pstrcmp(const char **a, const char **b)
+{
+    return g_strcmp0(*a, *b);
+}
+
 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)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);
+
 }
 /* ------------------------------------------------------------------ */
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 03/10] qom/object: fix iterating properties over a class

[Marc-André Lureau]

object_class_property_iter_init() starts from the given class, so the
next class should continue with the parent class.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index 75d1d48944..d8666de3f2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1108,7 +1108,7 @@ void object_class_property_iter_init(ObjectPropertyIterator *iter,
                                      ObjectClass *klass)
 {
     g_hash_table_iter_init(&iter->iter, klass->properties);
-    iter->nextclass = klass;
+    iter->nextclass = object_class_get_parent(klass);
 }
 
 ObjectProperty *object_class_property_find(ObjectClass *klass, const char *name,
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 04/10] qom/object: register 'type' property as class property

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/object.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index d8666de3f2..185d1dd9f8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2423,9 +2423,10 @@ void object_class_property_set_description(ObjectClass *klass,
     op->description = g_strdup(description);
 }
 
-static void object_instance_init(Object *obj)
+static void object_class_init(ObjectClass *klass, void *data)
 {
-    object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
+    object_class_property_add_str(klass, "type", qdev_get_type,
+                                  NULL, &error_abort);
 }
 
 static void register_types(void)
@@ -2439,7 +2440,7 @@ static void register_types(void)
     static TypeInfo object_info = {
         .name = TYPE_OBJECT,
         .instance_size = sizeof(Object),
-        .instance_init = object_instance_init,
+        .class_init = object_class_init,
         .abstract = true,
     };
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 05/10] tests/qom-proplist: check duplicate "bv" property registration failed

[Marc-André Lureau]

"bv" is already a class property.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/check-qom-proplist.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 92898e1520..0f6d9c1ce3 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -125,10 +125,13 @@ static char *dummy_get_sv(Object *obj,
 
 static void dummy_init(Object *obj)
 {
+    Error *err = NULL;
+
     object_property_add_bool(obj, "bv",
                              dummy_get_bv,
                              dummy_set_bv,
-                             NULL);
+                             &err);
+    error_free_or_abort(&err);
 }
 
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 06/10] tests/qom-proplist: check properties are not listed multiple times

[Marc-André Lureau]

And factor out a common function used by the follow class properties
iterator test.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/check-qom-proplist.c | 44 +++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 0f6d9c1ce3..8e1b9c27f3 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -520,32 +520,19 @@ static void test_dummy_getenum(void)
 }
 
 
-static void test_dummy_iterator(void)
+static void test_dummy_prop_iterator(ObjectPropertyIterator *iter)
 {
-    Object *parent = object_get_objects_root();
-    DummyObject *dobj = DUMMY_OBJECT(
-        object_new_with_props(TYPE_DUMMY,
-                              parent,
-                              "dummy0",
-                              &error_abort,
-                              "bv", "yes",
-                              "sv", "Hiss hiss hiss",
-                              "av", "platypus",
-                              NULL));
-
-    ObjectProperty *prop;
-    ObjectPropertyIterator iter;
     bool seenbv = false, seensv = false, seenav = false, seentype;
+    ObjectProperty *prop;
 
-    object_property_iter_init(&iter, OBJECT(dobj));
-    while ((prop = object_property_iter_next(&iter))) {
-        if (g_str_equal(prop->name, "bv")) {
+    while ((prop = object_property_iter_next(iter))) {
+        if (!seenbv && g_str_equal(prop->name, "bv")) {
             seenbv = true;
-        } else if (g_str_equal(prop->name, "sv")) {
+        } else if (!seensv && g_str_equal(prop->name, "sv")) {
             seensv = true;
-        } else if (g_str_equal(prop->name, "av")) {
+        } else if (!seenav && g_str_equal(prop->name, "av")) {
             seenav = true;
-        } else if (g_str_equal(prop->name, "type")) {
+        } else if (!seentype && g_str_equal(prop->name, "type")) {
             /* This prop comes from the base Object class */
             seentype = true;
         } else {
@@ -557,7 +544,24 @@ static void test_dummy_iterator(void)
     g_assert(seenav);
     g_assert(seensv);
     g_assert(seentype);
+}
+
+static void test_dummy_iterator(void)
+{
+    Object *parent = object_get_objects_root();
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_with_props(TYPE_DUMMY,
+                              parent,
+                              "dummy0",
+                              &error_abort,
+                              "bv", "yes",
+                              "sv", "Hiss hiss hiss",
+                              "av", "platypus",
+                              NULL));
+    ObjectPropertyIterator iter;
 
+    object_property_iter_init(&iter, OBJECT(dobj));
+    test_dummy_prop_iterator(&iter);
     object_unparent(OBJECT(dobj));
 }
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 07/10] tests/qom-proplist: check class properties iterator

[Marc-André Lureau]

This test failed before "fix iterating properties over a class".

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/check-qom-proplist.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 8e1b9c27f3..7ed16b704b 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -565,6 +565,14 @@ static void test_dummy_iterator(void)
     object_unparent(OBJECT(dobj));
 }
 
+static void test_dummy_class_iterator(void)
+{
+    ObjectPropertyIterator iter;
+    ObjectClass *klass = object_class_by_name(TYPE_DUMMY);
+
+    object_class_property_iter_init(&iter, klass);
+    test_dummy_prop_iterator(&iter);
+}
 
 static void test_dummy_delchild(void)
 {
@@ -636,6 +644,7 @@ int main(int argc, char **argv)
     g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
     g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
     g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
+    g_test_add_func("/qom/proplist/class_iterator", test_dummy_class_iterator);
     g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
     g_test_add_func("/qom/resolve/partial", test_qom_partial_path);
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 08/10] vl: handle -object ?

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 vl.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/vl.c b/vl.c
index 5ba06adf78..8a5fd0c81f 100644
--- a/vl.c
+++ b/vl.c
@@ -2731,6 +2731,19 @@ static int machine_set_property(void *opaque,
  */
 static bool object_create_initial(const char *type)
 {
+    if (is_help_option(type)) {
+        GSList *l, *list;
+
+        error_printf("List of user creatable objects:\n");
+        list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
+        for (l = list; l != NULL; l = l->next) {
+            ObjectClass *oc = OBJECT_CLASS(l->data);
+            error_printf("%s\n", object_class_get_name(oc));
+        }
+        g_slist_free(list);
+        exit(0);
+    }
+
     if (g_str_equal(type, "rng-egd") ||
         g_str_has_prefix(type, "pr-manager-")) {
         return false;
-- 
2.19.0.rc1

[Qemu-devel] [PATCH 09/10] hostmem: add some properties description

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 backends/hostmem-memfd.c |  9 +++++++++
 backends/hostmem.c       | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index 1e20fe0ba8..789c8c3f87 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -144,14 +144,23 @@ memfd_backend_class_init(ObjectClass *oc, void *data)
                                    memfd_backend_get_hugetlb,
                                    memfd_backend_set_hugetlb,
                                    &error_abort);
+    object_class_property_set_description(oc, "hugetlb",
+                                          "Use huge pages",
+                                          &error_abort);
     object_class_property_add(oc, "hugetlbsize", "int",
                               memfd_backend_get_hugetlbsize,
                               memfd_backend_set_hugetlbsize,
                               NULL, NULL, &error_abort);
+    object_class_property_set_description(oc, "hugetlbsize",
+                                          "Huge pages size (ex: 2M, 1G)",
+                                          &error_abort);
     object_class_property_add_bool(oc, "seal",
                                    memfd_backend_get_seal,
                                    memfd_backend_set_seal,
                                    &error_abort);
+    object_class_property_set_description(oc, "seal",
+                                          "Seal growing & shrinking",
+                                          &error_abort);
 }
 
 static const TypeInfo memfd_backend_info = {
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4908946cd3..1a89342039 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -397,27 +397,41 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, "merge",
         host_memory_backend_get_merge,
         host_memory_backend_set_merge, &error_abort);
+    object_class_property_set_description(oc, "merge",
+        "Mark memory as mergeable", &error_abort);
     object_class_property_add_bool(oc, "dump",
         host_memory_backend_get_dump,
         host_memory_backend_set_dump, &error_abort);
+    object_class_property_set_description(oc, "dump",
+        "Set to 'off' to exclude from core dump", &error_abort);
     object_class_property_add_bool(oc, "prealloc",
         host_memory_backend_get_prealloc,
         host_memory_backend_set_prealloc, &error_abort);
+    object_class_property_set_description(oc, "prealloc",
+        "Preallocate memory", &error_abort);
     object_class_property_add(oc, "size", "int",
         host_memory_backend_get_size,
         host_memory_backend_set_size,
         NULL, NULL, &error_abort);
+    object_class_property_set_description(oc, "size",
+        "Size of the memory region (ex: 500M)", &error_abort);
     object_class_property_add(oc, "host-nodes", "int",
         host_memory_backend_get_host_nodes,
         host_memory_backend_set_host_nodes,
         NULL, NULL, &error_abort);
+    object_class_property_set_description(oc, "host-nodes",
+        "Binds memory to the list of NUMA host nodes", &error_abort);
     object_class_property_add_enum(oc, "policy", "HostMemPolicy",
         &HostMemPolicy_lookup,
         host_memory_backend_get_policy,
         host_memory_backend_set_policy, &error_abort);
+    object_class_property_set_description(oc, "policy",
+        "Set the NUMA policy", &error_abort);
     object_class_property_add_bool(oc, "share",
         host_memory_backend_get_share, host_memory_backend_set_share,
         &error_abort);
+    object_class_property_set_description(oc, "share",
+        "Mark the memory as private to QEMU or shared", &error_abort);
 }
 
 static const TypeInfo host_memory_backend_info = {
-- 
2.19.0.rc1

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

[Marc-André Lureau]

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

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);
 }
 
 
-- 
2.19.0.rc1

Re: [Qemu-devel] [PATCH 01/10] qemu-option: add help fallback to print the list of options

[Eric Blake]

On 09/06/2018 10:12 AM, Marc-André Lureau wrote:
> QDev options accept '?' or 'help' in the list of parameters, which is
> really handy to list the available options.
> 
> Unfortunately, this isn't built in QemuOpts. qemu_opts_parse_noisily()
> seems to be the common path for command line options, so place a
> fallback to check for '?' and print help listing available options.
> 
> This is very handy, for example with qemu "-spice ?".

Is that literal spelling intended (a single argument with an embedded 
space)? Because that would result in:
qemu: -spice help: invalid option

Or did you mean "qemu -spice '?'" (with the outer quotes delimiting what 
you type, and the inner quote properly escaping the ? to avoid 
unintended globbing if you have a one-byte file name in the current 
directory)?

Also, I'd rather see you favor the 'help' spelling in your text; the '?' 
spelling should continue to work (by virtue of has_help_option), but as 
that form requires shell quoting while 'help' does not, we should 
minimize its use in our documentation to avoid people forgetting to 
quote it and then hitting unintended shell globbing effects.  That would 
make your example "qemu -spice help".

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   util/qemu-option.c | 33 ++++++++++++++++++++++-----------
>   1 file changed, 22 insertions(+), 11 deletions(-)
> 
> @@ -886,10 +891,16 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
>   {
>       Error *err = NULL;
>       QemuOpts *opts;
> +    bool invalidp = false;
>   
> -    opts = opts_parse(list, params, permit_abbrev, false, &err);
> +    opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
>       if (err) {
> -        error_report_err(err);
> +        if (invalidp && has_help_option(params)) {
> +            qemu_opts_print_help(list);
> +            error_free(err);
> +        } else {
> +            error_report_err(err);
> +        }

This makes sense.  With a better commit message,
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH 02/10] qemu-option: improve qemu_opts_print_help() output

[Eric Blake]

On 09/06/2018 10:12 AM, 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)
> - drop 16-chars alignment, use a '-' as seperator for option name and
>    description
> 
> For ex, "-spice ?" output is changed from:

s/?/help/

> 
> 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 ?", changed from:

and again

> 
> 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.lureau@redhat.com>
> ---
>   util/qemu-option.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 42 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH 08/10] vl: handle -object ?

[Eric Blake]

On 09/06/2018 10:12 AM, Marc-André Lureau wrote:

In the subject, s/\?/help/

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   vl.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 5ba06adf78..8a5fd0c81f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2731,6 +2731,19 @@ static int machine_set_property(void *opaque,
>    */
>   static bool object_create_initial(const char *type)
>   {
> +    if (is_help_option(type)) {
> +        GSList *l, *list;
> +
> +        error_printf("List of user creatable objects:\n");
> +        list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
> +        for (l = list; l != NULL; l = l->next) {
> +            ObjectClass *oc = OBJECT_CLASS(l->data);
> +            error_printf("%s\n", object_class_get_name(oc));

This prints to stderr,

> +        }
> +        g_slist_free(list);
> +        exit(0);

then exits with status 0.  I'd much rather successful help text be 
printed to stdout.

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

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

[Eric Blake]

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.

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

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


-- 
Marc-André Lureau

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

[Eric Blake]

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

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