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

[Qemu-devel] [PATCH v2 00/12] 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 "qemu-option" 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 related patches for QOM,
to fix/improve some minor issues.

v2: after Eric Blake review
- add "qdev-monitor: print help to stdout"
- add "cutils: add qemu_pstrcmp"
- use consistently "arg=type - desc" help format

Marc-André Lureau (12):
  qdev-monitor: print help to stdout
  cutils: add qemu_pstrcmp
  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 help
  hostmem: add some properties description
  vl: list user creatable properties when 'help' is argument

 include/monitor/monitor.h  |  2 ++
 include/qemu/cutils.h      | 12 +++++++
 backends/hostmem-memfd.c   |  9 +++++
 backends/hostmem.c         | 14 ++++++++
 monitor.c                  | 16 +++++++--
 qdev-monitor.c             | 32 ++++++++++-------
 qom/object.c               |  9 ++---
 qom/object_interfaces.c    |  6 ++--
 tests/check-qom-proplist.c | 58 ++++++++++++++++++++-----------
 util/cutils.c              |  5 +++
 util/qemu-option.c         | 71 +++++++++++++++++++++++++++++++-------
 vl.c                       | 53 ++++++++++++++++++++++++++--
 12 files changed, 227 insertions(+), 60 deletions(-)

-- 
2.19.0.rc1

[Qemu-devel] [PATCH v2 01/12] qdev-monitor: print help to stdout

[Marc-André Lureau]

qdev_device_help() is used from command line "-device help", or from
HMP "device_add". If used from command line, print help to stdout
(it is only printed on explicit demand).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/monitor/monitor.h |  2 ++
 monitor.c                 | 16 +++++++++++++---
 qdev-monitor.c            | 32 +++++++++++++++++++-------------
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 2ef5e04b37..e7667fda35 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -47,4 +47,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
 void monitor_fdset_dup_fd_remove(int dup_fd);
 int monitor_fdset_dup_fd_find(int dup_fd);
 
+void out_vprintf(FILE *stream, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
+
 #endif /* MONITOR_H */
diff --git a/monitor.c b/monitor.c
index 021c11b1bf..6ea4082ab5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4597,19 +4597,29 @@ static void monitor_readline_flush(void *opaque)
 }
 
 /*
- * Print to current monitor if we have one, else to stderr.
+ * Print to current monitor if we have one, else to stream.
  * TODO should return int, so callers can calculate width, but that
  * requires surgery to monitor_vprintf().  Left for another day.
  */
-void error_vprintf(const char *fmt, va_list ap)
+void out_vprintf(FILE *stream, const char *fmt, va_list ap)
 {
     if (cur_mon && !monitor_cur_is_qmp()) {
         monitor_vprintf(cur_mon, fmt, ap);
     } else {
-        vfprintf(stderr, fmt, ap);
+        vfprintf(stream, fmt, ap);
     }
 }
 
+/*
+ * Print to current monitor if we have one, else to stderr.
+ * TODO should return int, so callers can calculate width, but that
+ * requires surgery to monitor_vprintf().  Left for another day.
+ */
+void error_vprintf(const char *fmt, va_list ap)
+{
+    out_vprintf(stderr, fmt, ap);
+}
+
 void error_vprintf_unless_qmp(const char *fmt, va_list ap)
 {
     if (cur_mon && !monitor_cur_is_qmp()) {
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 61e0300991..1da324aba6 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -104,22 +104,31 @@ static bool qdev_class_has_alias(DeviceClass *dc)
     return (qdev_class_get_alias(dc) != NULL);
 }
 
+static void out_printf(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    out_vprintf(stdout, fmt, ap);
+    va_end(ap);
+}
+
 static void qdev_print_devinfo(DeviceClass *dc)
 {
-    error_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
+    out_printf("name \"%s\"", object_class_get_name(OBJECT_CLASS(dc)));
     if (dc->bus_type) {
-        error_printf(", bus %s", dc->bus_type);
+        out_printf(", bus %s", dc->bus_type);
     }
     if (qdev_class_has_alias(dc)) {
-        error_printf(", alias \"%s\"", qdev_class_get_alias(dc));
+        out_printf(", alias \"%s\"", qdev_class_get_alias(dc));
     }
     if (dc->desc) {
-        error_printf(", desc \"%s\"", dc->desc);
+        out_printf(", desc \"%s\"", dc->desc);
     }
     if (!dc->user_creatable) {
-        error_printf(", no-user");
+        out_printf(", no-user");
     }
-    error_printf("\n");
+    out_printf("\n");
 }
 
 static void qdev_print_devinfos(bool show_no_user)
@@ -155,8 +164,7 @@ static void qdev_print_devinfos(bool show_no_user)
                 continue;
             }
             if (!cat_printed) {
-                error_printf("%s%s devices:\n", i ? "\n" : "",
-                             cat_name[i]);
+                out_printf("%s%s devices:\n", i ? "\n" : "", cat_name[i]);
                 cat_printed = true;
             }
             qdev_print_devinfo(dc);
@@ -278,13 +286,11 @@ int qdev_device_help(QemuOpts *opts)
     }
 
     for (prop = prop_list; prop; prop = prop->next) {
-        error_printf("%s.%s=%s", driver,
-                     prop->value->name,
-                     prop->value->type);
+        out_printf("%s.%s=%s", driver, prop->value->name, prop->value->type);
         if (prop->value->has_description) {
-            error_printf(" (%s)\n", prop->value->description);
+            out_printf(" (%s)\n", prop->value->description);
         } else {
-            error_printf("\n");
+            out_printf("\n");
         }
     }
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH v2 02/12] cutils: add qemu_pstrcmp

[Marc-André Lureau]

A char** variant of strcmp().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/cutils.h | 12 ++++++++++++
 util/cutils.c         |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 47aaa3b0b9..0941639f36 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -169,4 +169,16 @@ bool test_buffer_is_zero_next_accel(void);
 int uleb128_encode_small(uint8_t *out, uint32_t n);
 int uleb128_decode_small(const uint8_t *in, uint32_t *n);
 
+/**
+ * qemu_pstrcmp:
+ * @str1: a pointer to a C string
+ * @str2: a pointer to a C string
+ *
+ * Compares *str1 and *str2 with g_strcmp0().
+ *
+ * Returns: an integer less than, equal to, or greater than zero, if
+ * *str1 is <, == or > than *str2 .
+ */
+int qemu_pstrcmp(const char **str1, const char **str2);
+
 #endif
diff --git a/util/cutils.c b/util/cutils.c
index 9205e09031..e2abc64278 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -769,3 +769,8 @@ char *size_to_str(uint64_t val)
 
     return g_strdup_printf("%0.3g %sB", (double)val / div, suffixes[i]);
 }
+
+int qemu_pstrcmp(const char **str1, const char **str2)
+{
+    return g_strcmp0(*str1, *str2);
+}
-- 
2.19.0.rc1

[Qemu-devel] [PATCH v2 03/12] qemu-option: add help fallback to print the list of options

[Marc-André Lureau]

QDev options accept 'help' (or '?', but that's problematic with shell
globing) in the list of parameters, which is 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 print help, listing the available options.

This is quite handy, for example with qemu "-spice help".

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@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 v2 04/12] 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 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.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@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);
+
 }
 /* ------------------------------------------------------------------ */
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH v2 05/12] 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 v2 06/12] qom/object: register 'type' property as class property

[Marc-André Lureau]

Let's save a few byte in each object instance.

(Is this property really used anywhere? Only qom-get could read it)

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 v2 07/12] 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 v2 08/12] 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 v2 09/12] 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 v2 10/12] vl: handle -object help

[Marc-André Lureau]

List the user creatable objects.

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..71765a2982 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;
+
+        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);
+            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 v2 11/12] 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 v2 12/12] vl: list user creatable properties when 'help' is 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-file,help
memory-backend-file.align=int
memory-backend-file.discard-data=bool
memory-backend-file.dump=bool - Set to 'off' to exclude from core dump
memory-backend-file.host-nodes=int - Binds memory to the list of NUMA host nodes
memory-backend-file.mem-path=string
memory-backend-file.merge=bool - Mark memory as mergeable
memory-backend-file.pmem=bool
memory-backend-file.policy=HostMemPolicy - Set the NUMA policy
memory-backend-file.prealloc=bool - Preallocate memory
memory-backend-file.share=bool - Mark the memory as private to QEMU or shared
memory-backend-file.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                    | 40 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 40 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 71765a2982..880676ecc1 100644
--- a/vl.c
+++ b/vl.c
@@ -2729,8 +2729,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 +2746,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)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);
+        exit(0);
+    }
+
     if (g_str_equal(type, "rng-egd") ||
         g_str_has_prefix(type, "pr-manager-")) {
         return false;
@@ -2790,9 +2824,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 v2 01/12] qdev-monitor: print help to stdout

[Thomas Huth]

On 2018-09-07 09:59, Marc-André Lureau wrote:
> qdev_device_help() is used from command line "-device help", or from
> HMP "device_add". If used from command line, print help to stdout
> (it is only printed on explicit demand).

Good idea, it always bugged me that "-device help" behaves differently
than "-help"!

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/monitor/monitor.h |  2 ++
>  monitor.c                 | 16 +++++++++++++---
>  qdev-monitor.c            | 32 +++++++++++++++++++-------------
>  3 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 2ef5e04b37..e7667fda35 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -47,4 +47,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
>  void monitor_fdset_dup_fd_remove(int dup_fd);
>  int monitor_fdset_dup_fd_find(int dup_fd);
>  
> +void out_vprintf(FILE *stream, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);

The name is a little bit too generic for my taste ... but I also fail to
come up with really good suggestions... maybe "mon_file_vprintf()" or
something similar?

 Thomas

Re: [Qemu-devel] [PATCH v2 02/12] cutils: add qemu_pstrcmp

[Thomas Huth]

On 2018-09-07 09:59, Marc-André Lureau wrote:
> A char** variant of strcmp().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/cutils.h | 12 ++++++++++++
>  util/cutils.c         |  5 +++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 47aaa3b0b9..0941639f36 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -169,4 +169,16 @@ bool test_buffer_is_zero_next_accel(void);
>  int uleb128_encode_small(uint8_t *out, uint32_t n);
>  int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>  
> +/**
> + * qemu_pstrcmp:
> + * @str1: a pointer to a C string
> + * @str2: a pointer to a C string
> + *
> + * Compares *str1 and *str2 with g_strcmp0().
> + *
> + * Returns: an integer less than, equal to, or greater than zero, if
> + * *str1 is <, == or > than *str2 .
> + */

Maybe add a comment that str1 or str2 can also be NULL?

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 02/12] cutils: add qemu_pstrcmp

[Marc-André Lureau]

Hi

On Fri, Sep 7, 2018 at 12:26 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 2018-09-07 09:59, Marc-André Lureau wrote:
> > A char** variant of strcmp().
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/qemu/cutils.h | 12 ++++++++++++
> >  util/cutils.c         |  5 +++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> > index 47aaa3b0b9..0941639f36 100644
> > --- a/include/qemu/cutils.h
> > +++ b/include/qemu/cutils.h
> > @@ -169,4 +169,16 @@ bool test_buffer_is_zero_next_accel(void);
> >  int uleb128_encode_small(uint8_t *out, uint32_t n);
> >  int uleb128_decode_small(const uint8_t *in, uint32_t *n);
> >
> > +/**
> > + * qemu_pstrcmp:
> > + * @str1: a pointer to a C string
> > + * @str2: a pointer to a C string
> > + *
> > + * Compares *str1 and *str2 with g_strcmp0().
> > + *
> > + * Returns: an integer less than, equal to, or greater than zero, if
> > + * *str1 is <, == or > than *str2 .
> > + */
>
> Maybe add a comment that str1 or str2 can also be NULL?

*str1 or *str2 (not str1 or str2). ok

>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 02/12] cutils: add qemu_pstrcmp

[Thomas Huth]

On 2018-09-07 10:29, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 7, 2018 at 12:26 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 2018-09-07 09:59, Marc-André Lureau wrote:
>>> A char** variant of strcmp().
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  include/qemu/cutils.h | 12 ++++++++++++
>>>  util/cutils.c         |  5 +++++
>>>  2 files changed, 17 insertions(+)
>>>
>>> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
>>> index 47aaa3b0b9..0941639f36 100644
>>> --- a/include/qemu/cutils.h
>>> +++ b/include/qemu/cutils.h
>>> @@ -169,4 +169,16 @@ bool test_buffer_is_zero_next_accel(void);
>>>  int uleb128_encode_small(uint8_t *out, uint32_t n);
>>>  int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>>>
>>> +/**
>>> + * qemu_pstrcmp:
>>> + * @str1: a pointer to a C string
>>> + * @str2: a pointer to a C string
>>> + *
>>> + * Compares *str1 and *str2 with g_strcmp0().
>>> + *
>>> + * Returns: an integer less than, equal to, or greater than zero, if
>>> + * *str1 is <, == or > than *str2 .
>>> + */
>>
>> Maybe add a comment that str1 or str2 can also be NULL?
> 
> *str1 or *str2 (not str1 or str2). ok

Ah, right, that's what I meant. Maybe clarify both conditions in the
comment ;-)

 Thomas

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

[Eric Blake]

On 09/07/2018 02:59 AM, Marc-André Lureau wrote:
> QDev options accept 'help' (or '?', but that's problematic with shell
> globing) in the list of parameters, which is handy to list the

s/globing/globbing/

> 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 print help, listing the available options.
> 
> This is quite handy, for example with qemu "-spice help".
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   util/qemu-option.c | 33 ++++++++++++++++++++++-----------
>   1 file changed, 22 insertions(+), 11 deletions(-)
> 

R-b still stands.

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

Re: [Qemu-devel] [PATCH v2 01/12] qdev-monitor: print help to stdout

[Eric Blake]

On 09/07/2018 02:59 AM, Marc-André Lureau wrote:
> qdev_device_help() is used from command line "-device help", or from
> HMP "device_add". If used from command line, print help to stdout
> (it is only printed on explicit demand).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/monitor/monitor.h |  2 ++
>   monitor.c                 | 16 +++++++++++++---
>   qdev-monitor.c            | 32 +++++++++++++++++++-------------
>   3 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 2ef5e04b37..e7667fda35 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -47,4 +47,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
>   void monitor_fdset_dup_fd_remove(int dup_fd);
>   int monitor_fdset_dup_fd_find(int dup_fd);
>   
> +void out_vprintf(FILE *stream, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);

Perhaps name this monitor_vfprintf()?  (That would match the fact that 
you are using it like normal vfprintf).

> +++ b/qdev-monitor.c
> @@ -104,22 +104,31 @@ static bool qdev_class_has_alias(DeviceClass *dc)
>       return (qdev_class_get_alias(dc) != NULL);
>   }
>   
> +static void out_printf(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    out_vprintf(stdout, fmt, ap);
> +    va_end(ap);
> +}

But this name seems reasonable.

And I just now see that Thomas also complained about the monitor.h 
naming, but I like 'monitor_vfprintf' better than his 'mon_file_vprintf'.

If an improved name is your only change,
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 v2 12/12] vl: list user creatable properties when 'help' is argument

[Eric Blake]

On 09/07/2018 02:59 AM, Marc-André Lureau wrote:
> Iterate over the writable class properties, sort and print them out
> with the description if available.
> 
> Ex: qemu -object memory-backend-file,help
> memory-backend-file.align=int
> memory-backend-file.discard-data=bool
> memory-backend-file.dump=bool - Set to 'off' to exclude from core dump
> memory-backend-file.host-nodes=int - Binds memory to the list of NUMA host nodes
> memory-backend-file.mem-path=string
> memory-backend-file.merge=bool - Mark memory as mergeable
> memory-backend-file.pmem=bool
> memory-backend-file.policy=HostMemPolicy - Set the NUMA policy
> memory-backend-file.prealloc=bool - Preallocate memory
> memory-backend-file.share=bool - Mark the memory as private to QEMU or shared
> memory-backend-file.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                    | 40 +++++++++++++++++++++++++++++++++++++---
>   2 files changed, 40 insertions(+), 6 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 v2 01/12] qdev-monitor: print help to stdout

[Marc-André Lureau]

Hi

On Fri, Sep 7, 2018 at 5:49 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 09/07/2018 02:59 AM, Marc-André Lureau wrote:
> > qdev_device_help() is used from command line "-device help", or from
> > HMP "device_add". If used from command line, print help to stdout
> > (it is only printed on explicit demand).
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   include/monitor/monitor.h |  2 ++
> >   monitor.c                 | 16 +++++++++++++---
> >   qdev-monitor.c            | 32 +++++++++++++++++++-------------
> >   3 files changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 2ef5e04b37..e7667fda35 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -47,4 +47,6 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> >   void monitor_fdset_dup_fd_remove(int dup_fd);
> >   int monitor_fdset_dup_fd_find(int dup_fd);
> >
> > +void out_vprintf(FILE *stream, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
>
> Perhaps name this monitor_vfprintf()?  (That would match the fact that
> you are using it like normal vfprintf).
>
> > +++ b/qdev-monitor.c
> > @@ -104,22 +104,31 @@ static bool qdev_class_has_alias(DeviceClass *dc)
> >       return (qdev_class_get_alias(dc) != NULL);
> >   }
> >
> > +static void out_printf(const char *fmt, ...)
> > +{
> > +    va_list ap;
> > +
> > +    va_start(ap, fmt);
> > +    out_vprintf(stdout, fmt, ap);
> > +    va_end(ap);
> > +}
>
> But this name seems reasonable.
>
> And I just now see that Thomas also complained about the monitor.h
> naming, but I like 'monitor_vfprintf' better than his 'mon_file_vprintf'.
>
> If an improved name is your only change,
> Reviewed-by: Eric Blake <eblake@redhat.com>

I'll change it to monitor_vfprintf,
thanks

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


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 00/12] Various qemu command line options help improvements

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> Hi,
> 
> This is a compilation of patches I have to improve command line help
> support. The "qemu-option" 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 related patches for QOM,
> to fix/improve some minor issues.
> 
> v2: after Eric Blake review
> - add "qdev-monitor: print help to stdout"
> - add "cutils: add qemu_pstrcmp"
> - use consistently "arg=type - desc" help format

Feel free to send a pull request yourself with the minor fixes requested
during review.

Paolo

> Marc-André Lureau (12):
>   qdev-monitor: print help to stdout
>   cutils: add qemu_pstrcmp
>   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 help
>   hostmem: add some properties description
>   vl: list user creatable properties when 'help' is argument
> 
>  include/monitor/monitor.h  |  2 ++
>  include/qemu/cutils.h      | 12 +++++++
>  backends/hostmem-memfd.c   |  9 +++++
>  backends/hostmem.c         | 14 ++++++++
>  monitor.c                  | 16 +++++++--
>  qdev-monitor.c             | 32 ++++++++++-------
>  qom/object.c               |  9 ++---
>  qom/object_interfaces.c    |  6 ++--
>  tests/check-qom-proplist.c | 58 ++++++++++++++++++++-----------
>  util/cutils.c              |  5 +++
>  util/qemu-option.c         | 71 +++++++++++++++++++++++++++++++-------
>  vl.c                       | 53 ++++++++++++++++++++++++++--
>  12 files changed, 227 insertions(+), 60 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 00/12] Various qemu command line options help improvements

[Marc-André Lureau]

Hi

On Tue, Sep 11, 2018 at 5:17 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 07/09/2018 09:59, Marc-André Lureau wrote:
> > Hi,
> >
> > This is a compilation of patches I have to improve command line help
> > support. The "qemu-option" 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 related patches for QOM,
> > to fix/improve some minor issues.
> >
> > v2: after Eric Blake review
> > - add "qdev-monitor: print help to stdout"
> > - add "cutils: add qemu_pstrcmp"
> > - use consistently "arg=type - desc" help format
>
> Feel free to send a pull request yourself with the minor fixes requested
> during review.

It would be nice to get some more review before :)

Andreas, as QOM maintainer, do you have time to take a look?

Anybody else interested to improve the CLI?

thanks

> Paolo
>
> > Marc-André Lureau (12):
> >   qdev-monitor: print help to stdout
> >   cutils: add qemu_pstrcmp
> >   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 help
> >   hostmem: add some properties description
> >   vl: list user creatable properties when 'help' is argument
> >
> >  include/monitor/monitor.h  |  2 ++
> >  include/qemu/cutils.h      | 12 +++++++
> >  backends/hostmem-memfd.c   |  9 +++++
> >  backends/hostmem.c         | 14 ++++++++
> >  monitor.c                  | 16 +++++++--
> >  qdev-monitor.c             | 32 ++++++++++-------
> >  qom/object.c               |  9 ++---
> >  qom/object_interfaces.c    |  6 ++--
> >  tests/check-qom-proplist.c | 58 ++++++++++++++++++++-----------
> >  util/cutils.c              |  5 +++
> >  util/qemu-option.c         | 71 +++++++++++++++++++++++++++++++-------
> >  vl.c                       | 53 ++++++++++++++++++++++++++--
> >  12 files changed, 227 insertions(+), 60 deletions(-)
> >
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 05/12] qom/object: fix iterating properties over a class

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> 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,
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 06/12] qom/object: register 'type' property as class property

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> Let's save a few byte in each object instance.
> 
> (Is this property really used anywhere? Only qom-get could read it)

Well, it's not really used but it's part of the external API.

> 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,
>      };
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 07/12] tests/qom-proplist: check duplicate "bv" property registration failed

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> "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);
>  }
>  
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 08/12] tests/qom-proplist: check properties are not listed multiple times

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> 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));
>  }
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 09/12] tests/qom-proplist: check class properties iterator

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> 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);
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 10/12] vl: handle -object help

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> List the user creatable objects.
> 
> 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..71765a2982 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;
> +
> +        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);
> +            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;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 11/12] hostmem: add some properties description

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> 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 = {
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 12/12] vl: list user creatable properties when 'help' is argument

[Paolo Bonzini]

On 07/09/2018 09:59, Marc-André Lureau wrote:
> Iterate over the writable class properties, sort and print them out
> with the description if available.
> 
> Ex: qemu -object memory-backend-file,help
> memory-backend-file.align=int
> memory-backend-file.discard-data=bool
> memory-backend-file.dump=bool - Set to 'off' to exclude from core dump
> memory-backend-file.host-nodes=int - Binds memory to the list of NUMA host nodes
> memory-backend-file.mem-path=string
> memory-backend-file.merge=bool - Mark memory as mergeable
> memory-backend-file.pmem=bool
> memory-backend-file.policy=HostMemPolicy - Set the NUMA policy
> memory-backend-file.prealloc=bool - Preallocate memory
> memory-backend-file.share=bool - Mark the memory as private to QEMU or shared
> memory-backend-file.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                    | 40 +++++++++++++++++++++++++++++++++++++---
>  2 files changed, 40 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 71765a2982..880676ecc1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2729,8 +2729,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 +2746,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)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);
> +        exit(0);
> +    }
> +
>      if (g_str_equal(type, "rng-egd") ||
>          g_str_has_prefix(type, "pr-manager-")) {
>          return false;
> @@ -2790,9 +2824,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);
>  }
>  
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 00/12] Various qemu command line options help improvements

[Paolo Bonzini]

On 02/10/2018 12:54, Marc-André Lureau wrote:
>>>
>>> This is a compilation of patches I have to improve command line help
>>> support. The "qemu-option" 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 related patches for QOM,
>>> to fix/improve some minor issues.
>>>
>>> v2: after Eric Blake review
>>> - add "qdev-monitor: print help to stdout"
>>> - add "cutils: add qemu_pstrcmp"
>>> - use consistently "arg=type - desc" help format
>> Feel free to send a pull request yourself with the minor fixes requested
>> during review.
> It would be nice to get some more review before :)

There you have it. :)

Paolo

> Andreas, as QOM maintainer, do you have time to take a look?
> 
> Anybody else interested to improve the CLI?

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6369 bytes --]

First of all, this patch broke iotest 082.  But then again, all that'd
be needed is a correction of the reference output.

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

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?

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)


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

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.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@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);
> +
>  }
>  /* ------------------------------------------------------------------ */
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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 <mreitz@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.lureau@redhat.com>
> > Reviewed-by: Eric Blake <eblake@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