[PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser

[PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser

[Kevin Wolf]

This replaces the QemuOpts-based help code for --object in the storage
daemon with code based on the keyval parser.

v2:
- Fixed double comma by reusing the existing key and value parsers [Eric]
- More tests to cover the additional cases

Kevin Wolf (4):
  keyval: Parse help options
  qom: Factor out helpers from user_creatable_print_help()
  qom: Add user_creatable_print_help_from_qdict()
  qemu-storage-daemon: Remove QemuOpts from --object parser

 include/qemu/option.h                |   2 +-
 include/qom/object_interfaces.h      |   9 ++
 qapi/qobject-input-visitor.c         |   2 +-
 qom/object_interfaces.c              |  99 ++++++++-----
 storage-daemon/qemu-storage-daemon.c |  15 +-
 tests/test-keyval.c                  | 205 +++++++++++++++++++--------
 util/keyval.c                        |  38 ++++-
 7 files changed, 252 insertions(+), 118 deletions(-)

-- 
2.25.4

[PATCH v2 1/4] keyval: Parse help options

[Kevin Wolf]

This adds a new parameter 'help' to keyval_parse() that enables parsing
of help options. If NULL is passed, the function behaves the same as
before. But if a bool pointer is given, it contains the information
whether an option "help" without value was given (which would otherwise
either result in an error or be interpreted as the value for an implied
key).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/option.h                |   2 +-
 qapi/qobject-input-visitor.c         |   2 +-
 storage-daemon/qemu-storage-daemon.c |   2 +-
 tests/test-keyval.c                  | 205 +++++++++++++++++++--------
 util/keyval.c                        |  38 ++++-
 5 files changed, 179 insertions(+), 70 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 05e8a15c73..ac69352e0e 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -149,6 +149,6 @@ void qemu_opts_free(QemuOptsList *list);
 QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
 
 QDict *keyval_parse(const char *params, const char *implied_key,
-                    Error **errp);
+                    bool *help, Error **errp);
 
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f918a05e5f..7b184b50a7 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -757,7 +757,7 @@ Visitor *qobject_input_visitor_new_str(const char *str,
         assert(args);
         v = qobject_input_visitor_new(QOBJECT(args));
     } else {
-        args = keyval_parse(str, implied_key, errp);
+        args = keyval_parse(str, implied_key, NULL, errp);
         if (!args) {
             return NULL;
         }
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index e6157ff518..bb9cb740f0 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -278,7 +278,7 @@ static void process_options(int argc, char *argv[])
                 }
                 qemu_opts_del(opts);
 
-                args = keyval_parse(optarg, "qom-type", &error_fatal);
+                args = keyval_parse(optarg, "qom-type", NULL, &error_fatal);
                 user_creatable_add_dict(args, true, &error_fatal);
                 qobject_unref(args);
                 break;
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index e331a84149..83b65f04f7 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -27,27 +27,28 @@ static void test_keyval_parse(void)
     QDict *qdict, *sub_qdict;
     char long_key[129];
     char *params;
+    bool help;
 
     /* Nothing */
-    qdict = keyval_parse("", NULL, &error_abort);
+    qdict = keyval_parse("", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 0);
     qobject_unref(qdict);
 
     /* Empty key (qemu_opts_parse() accepts this) */
-    qdict = keyval_parse("=val", NULL, &err);
+    qdict = keyval_parse("=val", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Empty key fragment */
-    qdict = keyval_parse(".", NULL, &err);
+    qdict = keyval_parse(".", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("key.", NULL, &err);
+    qdict = keyval_parse("key.", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Invalid non-empty key (qemu_opts_parse() doesn't care) */
-    qdict = keyval_parse("7up=val", NULL, &err);
+    qdict = keyval_parse("7up=val", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
@@ -56,25 +57,25 @@ static void test_keyval_parse(void)
     long_key[127] = 'z';
     long_key[128] = 0;
     params = g_strdup_printf("k.%s=v", long_key);
-    qdict = keyval_parse(params + 2, NULL, &err);
+    qdict = keyval_parse(params + 2, NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Overlong key fragment */
-    qdict = keyval_parse(params, NULL, &err);
+    qdict = keyval_parse(params, NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
     g_free(params);
 
     /* Long key (qemu_opts_parse() accepts and truncates silently) */
     params = g_strdup_printf("k.%s=v", long_key + 1);
-    qdict = keyval_parse(params + 2, NULL, &error_abort);
+    qdict = keyval_parse(params + 2, NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, long_key + 1), ==, "v");
     qobject_unref(qdict);
 
     /* Long key fragment */
-    qdict = keyval_parse(params, NULL, &error_abort);
+    qdict = keyval_parse(params, NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     sub_qdict = qdict_get_qdict(qdict, "k");
     g_assert(sub_qdict);
@@ -84,25 +85,25 @@ static void test_keyval_parse(void)
     g_free(params);
 
     /* Crap after valid key */
-    qdict = keyval_parse("key[0]=val", NULL, &err);
+    qdict = keyval_parse("key[0]=val", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Multiple keys, last one wins */
-    qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, &error_abort);
+    qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 2);
     g_assert_cmpstr(qdict_get_try_str(qdict, "a"), ==, "3");
     g_assert_cmpstr(qdict_get_try_str(qdict, "b"), ==, "2,x");
     qobject_unref(qdict);
 
     /* Even when it doesn't in qemu_opts_parse() */
-    qdict = keyval_parse("id=foo,id=bar", NULL, &error_abort);
+    qdict = keyval_parse("id=foo,id=bar", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "bar");
     qobject_unref(qdict);
 
     /* Dotted keys */
-    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
+    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 2);
     sub_qdict = qdict_get_qdict(qdict, "a");
     g_assert(sub_qdict);
@@ -115,48 +116,48 @@ static void test_keyval_parse(void)
     qobject_unref(qdict);
 
     /* Inconsistent dotted keys */
-    qdict = keyval_parse("a.b=1,a=2", NULL, &err);
+    qdict = keyval_parse("a.b=1,a=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("a.b=1,a.b.c=2", NULL, &err);
+    qdict = keyval_parse("a.b=1,a.b.c=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Trailing comma is ignored */
-    qdict = keyval_parse("x=y,", NULL, &error_abort);
+    qdict = keyval_parse("x=y,", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, "y");
     qobject_unref(qdict);
 
     /* Except when it isn't */
-    qdict = keyval_parse(",", NULL, &err);
+    qdict = keyval_parse(",", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Value containing ,id= not misinterpreted as qemu_opts_parse() does */
-    qdict = keyval_parse("x=,,id=bar", NULL, &error_abort);
+    qdict = keyval_parse("x=,,id=bar", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "x"), ==, ",id=bar");
     qobject_unref(qdict);
 
     /* Anti-social ID is left to caller (qemu_opts_parse() rejects it) */
-    qdict = keyval_parse("id=666", NULL, &error_abort);
+    qdict = keyval_parse("id=666", NULL, NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     g_assert_cmpstr(qdict_get_try_str(qdict, "id"), ==, "666");
     qobject_unref(qdict);
 
     /* Implied value not supported (unlike qemu_opts_parse()) */
-    qdict = keyval_parse("an,noaus,noaus=", NULL, &err);
+    qdict = keyval_parse("an,noaus,noaus=", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Implied value, key "no" (qemu_opts_parse(): negated empty key) */
-    qdict = keyval_parse("no", NULL, &err);
+    qdict = keyval_parse("no", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Implied key */
-    qdict = keyval_parse("an,aus=off,noaus=", "implied", &error_abort);
+    qdict = keyval_parse("an,aus=off,noaus=", "implied", NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 3);
     g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "an");
     g_assert_cmpstr(qdict_get_try_str(qdict, "aus"), ==, "off");
@@ -164,7 +165,7 @@ static void test_keyval_parse(void)
     qobject_unref(qdict);
 
     /* Implied dotted key */
-    qdict = keyval_parse("val", "eins.zwei", &error_abort);
+    qdict = keyval_parse("val", "eins.zwei", NULL, &error_abort);
     g_assert_cmpuint(qdict_size(qdict), ==, 1);
     sub_qdict = qdict_get_qdict(qdict, "eins");
     g_assert(sub_qdict);
@@ -173,19 +174,100 @@ static void test_keyval_parse(void)
     qobject_unref(qdict);
 
     /* Implied key with empty value (qemu_opts_parse() accepts this) */
-    qdict = keyval_parse(",", "implied", &err);
+    qdict = keyval_parse(",", "implied", NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Likewise (qemu_opts_parse(): implied key with comma value) */
-    qdict = keyval_parse(",,,a=1", "implied", &err);
+    qdict = keyval_parse(",,,a=1", "implied", NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Empty key is not an implied key */
-    qdict = keyval_parse("=val", "implied", &err);
+    qdict = keyval_parse("=val", "implied", NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
+
+    /* "help" is only a help option if it has no value */
+    qdict = keyval_parse("help=on", NULL, &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "help"), ==, "on");
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* Double comma after "help" in an implied key is not a help option */
+    qdict = keyval_parse("help,,abc", "implied", &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* Without implied key and without value, it's an error */
+    qdict = keyval_parse("help,,abc", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* "help" as the only option */
+    qdict = keyval_parse("help", NULL, &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 0);
+    g_assert_true(help);
+    qobject_unref(qdict);
+
+    /* "help" as the first part of the key */
+    qdict = keyval_parse("help.abc", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* "help" as the last part of the key */
+    qdict = keyval_parse("abc.help", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* "help" is not a value for the implied key if &help is given */
+    qdict = keyval_parse("help", "implied", &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 0);
+    g_assert_true(help);
+    qobject_unref(qdict);
+
+    /* "help" is a value for the implied key when passing NULL for help */
+    qdict = keyval_parse("help", "implied", NULL, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help");
+    qobject_unref(qdict);
+
+    /* "help.abc" is a value for the implied key */
+    qdict = keyval_parse("help.abc", "implied", &help, &err);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help.abc");
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* "abc.help" is a value for the implied key */
+    qdict = keyval_parse("abc.help", "implied", &help, &err);
+    g_assert_cmpuint(qdict_size(qdict), ==, 1);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "abc.help");
+    g_assert_false(help);
+    qobject_unref(qdict);
+
+    /* "help" as the last part of the key */
+    qdict = keyval_parse("abc.help", NULL, &help, &err);
+    error_free_or_abort(&err);
+    g_assert(!qdict);
+
+    /* Other keys before and after help are still parsed normally */
+    qdict = keyval_parse("number=42,help,foo=bar", NULL, &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 2);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "number"), ==, "42");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
+    g_assert_true(help);
+    qobject_unref(qdict);
+
+    /* ...even with an implied key */
+    qdict = keyval_parse("val,help,foo=bar", "implied", &help, &error_abort);
+    g_assert_cmpuint(qdict_size(qdict), ==, 2);
+    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val");
+    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
+    g_assert_true(help);
+    qobject_unref(qdict);
 }
 
 static void check_list012(QList *qlist)
@@ -210,26 +292,26 @@ static void test_keyval_parse_list(void)
     QDict *qdict, *sub_qdict;
 
     /* Root can't be a list */
-    qdict = keyval_parse("0=1", NULL, &err);
+    qdict = keyval_parse("0=1", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* List elements need not be in order */
-    qdict = keyval_parse("list.0=null,list.2=zwei,list.1=eins",
-                         NULL, &error_abort);
+    qdict = keyval_parse("list.0=null,list.2=zwei,list.1=eins", NULL, NULL,
+                         &error_abort);
     g_assert_cmpint(qdict_size(qdict), ==, 1);
     check_list012(qdict_get_qlist(qdict, "list"));
     qobject_unref(qdict);
 
     /* Multiple indexes, last one wins */
     qdict = keyval_parse("list.1=goner,list.0=null,list.01=eins,list.2=zwei",
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     g_assert_cmpint(qdict_size(qdict), ==, 1);
     check_list012(qdict_get_qlist(qdict, "list"));
     qobject_unref(qdict);
 
     /* List at deeper nesting */
-    qdict = keyval_parse("a.list.1=eins,a.list.00=null,a.list.2=zwei",
+    qdict = keyval_parse("a.list.1=eins,a.list.00=null,a.list.2=zwei", NULL,
                          NULL, &error_abort);
     g_assert_cmpint(qdict_size(qdict), ==, 1);
     sub_qdict = qdict_get_qdict(qdict, "a");
@@ -238,18 +320,19 @@ static void test_keyval_parse_list(void)
     qobject_unref(qdict);
 
     /* Inconsistent dotted keys: both list and dictionary */
-    qdict = keyval_parse("a.b.c=1,a.b.0=2", NULL, &err);
+    qdict = keyval_parse("a.b.c=1,a.b.0=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("a.0.c=1,a.b.c=2", NULL, &err);
+    qdict = keyval_parse("a.0.c=1,a.b.c=2", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 
     /* Missing list indexes */
-    qdict = keyval_parse("list.1=lonely", NULL, &err);
+    qdict = keyval_parse("list.1=lonely", NULL, NULL, &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
-    qdict = keyval_parse("list.0=null,list.2=eins,list.02=zwei", NULL, &err);
+    qdict = keyval_parse("list.0=null,list.2=eins,list.02=zwei", NULL, NULL,
+                         &err);
     error_free_or_abort(&err);
     g_assert(!qdict);
 }
@@ -261,7 +344,7 @@ static void test_keyval_visit_bool(void)
     QDict *qdict;
     bool b;
 
-    qdict = keyval_parse("bool1=on,bool2=off", NULL, &error_abort);
+    qdict = keyval_parse("bool1=on,bool2=off", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -273,7 +356,7 @@ static void test_keyval_visit_bool(void)
     visit_end_struct(v, NULL);
     visit_free(v);
 
-    qdict = keyval_parse("bool1=offer", NULL, &error_abort);
+    qdict = keyval_parse("bool1=offer", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -291,7 +374,7 @@ static void test_keyval_visit_number(void)
     uint64_t u;
 
     /* Lower limit zero */
-    qdict = keyval_parse("number1=0", NULL, &error_abort);
+    qdict = keyval_parse("number1=0", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -302,7 +385,7 @@ static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Upper limit 2^64-1 */
-    qdict = keyval_parse("number1=18446744073709551615,number2=-1",
+    qdict = keyval_parse("number1=18446744073709551615,number2=-1", NULL,
                          NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
@@ -316,8 +399,8 @@ static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Above upper limit */
-    qdict = keyval_parse("number1=18446744073709551616",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=18446744073709551616", NULL, NULL,
+                         &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -327,8 +410,8 @@ static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Below lower limit */
-    qdict = keyval_parse("number1=-18446744073709551616",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=-18446744073709551616", NULL, NULL,
+                         &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -338,8 +421,7 @@ static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Hex and octal */
-    qdict = keyval_parse("number1=0x2a,number2=052",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=0x2a,number2=052", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -352,8 +434,7 @@ static void test_keyval_visit_number(void)
     visit_free(v);
 
     /* Trailing crap */
-    qdict = keyval_parse("number1=3.14,number2=08",
-                         NULL, &error_abort);
+    qdict = keyval_parse("number1=3.14,number2=08", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -373,7 +454,7 @@ static void test_keyval_visit_size(void)
     uint64_t sz;
 
     /* Lower limit zero */
-    qdict = keyval_parse("sz1=0", NULL, &error_abort);
+    qdict = keyval_parse("sz1=0", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -389,7 +470,7 @@ static void test_keyval_visit_size(void)
     qdict = keyval_parse("sz1=9007199254740991,"
                          "sz2=9007199254740992,"
                          "sz3=9007199254740993",
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -406,7 +487,7 @@ static void test_keyval_visit_size(void)
     /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
     qdict = keyval_parse("sz1=9223372036854774784," /* 7ffffffffffffc00 */
                          "sz2=9223372036854775295", /* 7ffffffffffffdff */
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -421,7 +502,7 @@ static void test_keyval_visit_size(void)
     /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
     qdict = keyval_parse("sz1=18446744073709549568," /* fffffffffffff800 */
                          "sz2=18446744073709550591", /* fffffffffffffbff */
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -436,7 +517,7 @@ static void test_keyval_visit_size(void)
     /* Beyond limits */
     qdict = keyval_parse("sz1=-1,"
                          "sz2=18446744073709550592", /* fffffffffffffc00 */
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -449,7 +530,7 @@ static void test_keyval_visit_size(void)
 
     /* Suffixes */
     qdict = keyval_parse("sz1=8b,sz2=1.5k,sz3=2M,sz4=0.1G,sz5=16777215T",
-                         NULL, &error_abort);
+                         NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -468,7 +549,7 @@ static void test_keyval_visit_size(void)
     visit_free(v);
 
     /* Beyond limit with suffix */
-    qdict = keyval_parse("sz1=16777216T", NULL, &error_abort);
+    qdict = keyval_parse("sz1=16777216T", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -478,7 +559,7 @@ static void test_keyval_visit_size(void)
     visit_free(v);
 
     /* Trailing crap */
-    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, &error_abort);
+    qdict = keyval_parse("sz1=0Z,sz2=16Gi", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -497,7 +578,7 @@ static void test_keyval_visit_dict(void)
     QDict *qdict;
     int64_t i;
 
-    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, &error_abort);
+    qdict = keyval_parse("a.b.c=1,a.b.c=2,d=3", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -515,7 +596,7 @@ static void test_keyval_visit_dict(void)
     visit_end_struct(v, NULL);
     visit_free(v);
 
-    qdict = keyval_parse("a.b=", NULL, &error_abort);
+    qdict = keyval_parse("a.b=", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -537,7 +618,7 @@ static void test_keyval_visit_list(void)
     QDict *qdict;
     char *s;
 
-    qdict = keyval_parse("a.0=,a.1=I,a.2.0=II", NULL, &error_abort);
+    qdict = keyval_parse("a.0=,a.1=I,a.2.0=II", NULL, NULL, &error_abort);
     /* TODO empty list */
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
@@ -561,7 +642,7 @@ static void test_keyval_visit_list(void)
     visit_end_struct(v, NULL);
     visit_free(v);
 
-    qdict = keyval_parse("a.0=,b.0.0=head", NULL, &error_abort);
+    qdict = keyval_parse("a.0=,b.0.0=head", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -590,7 +671,7 @@ static void test_keyval_visit_optional(void)
     bool present;
     int64_t i;
 
-    qdict = keyval_parse("a.b=1", NULL, &error_abort);
+    qdict = keyval_parse("a.b=1", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -626,7 +707,7 @@ static void test_keyval_visit_alternate(void)
      * the string variant if there is one, else an error.
      * TODO make it work for unambiguous cases like AltEnumBool below
      */
-    qdict = keyval_parse("a=1,b=2,c=on", NULL, &error_abort);
+    qdict = keyval_parse("a=1,b=2,c=on", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
@@ -650,7 +731,7 @@ static void test_keyval_visit_any(void)
     QList *qlist;
     QString *qstr;
 
-    qdict = keyval_parse("a.0=null,a.1=1", NULL, &error_abort);
+    qdict = keyval_parse("a.0=null,a.1=1", NULL, NULL, &error_abort);
     v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     qobject_unref(qdict);
     visit_start_struct(v, NULL, NULL, 0, &error_abort);
diff --git a/util/keyval.c b/util/keyval.c
index 13def4af54..c8ce3bb6c9 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -166,7 +166,7 @@ static QObject *keyval_parse_put(QDict *cur,
  * On failure, return NULL.
  */
 static const char *keyval_parse_one(QDict *qdict, const char *params,
-                                    const char *implied_key,
+                                    const char *implied_key, bool *help,
                                     Error **errp)
 {
     const char *key, *key_end, *s, *end;
@@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
     if (key == implied_key) {
         assert(!*s);
         s = params;
+    } else if (*s == '=') {
+        s++;
     } else {
-        if (*s != '=') {
+        if (help && !strncmp(key, "help", s - key)) {
+            *help = true;
+            if (*s) {
+                s++;
+            }
+            return s;
+        } else {
             error_setg(errp, "Expected '=' after parameter '%.*s'",
                        (int)(s - key), key);
             return NULL;
         }
-        s++;
     }
 
     val = qstring_new();
@@ -260,6 +267,15 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
         qstring_append_chr(val, *s++);
     }
 
+    if (help && key == implied_key) {
+        const char *str_val = qstring_get_str(val);
+        if (!strcmp(str_val, "help")) {
+            *help = true;
+            qobject_unref(val);
+            return s;
+        }
+    }
+
     if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
         return NULL;
     }
@@ -388,21 +404,33 @@ static QObject *keyval_listify(QDict *cur, GSList *key_of_cur, Error **errp)
 
 /*
  * Parse @params in QEMU's traditional KEY=VALUE,... syntax.
+ *
  * If @implied_key, the first KEY= can be omitted.  @implied_key is
  * implied then, and VALUE can't be empty or contain ',' or '='.
+ *
+ * If @help is given, an option "help" without a value isn't added to
+ * the resulting dictionary, but instead sets @help to true. If no
+ * help option is found, @help is false on return. All other options
+ * are parsed and returned normally so that context specific help can
+ * be printed.
+ *
  * On success, return a dictionary of the parsed keys and values.
  * On failure, store an error through @errp and return NULL.
  */
 QDict *keyval_parse(const char *params, const char *implied_key,
-                    Error **errp)
+                    bool *help, Error **errp)
 {
     QDict *qdict = qdict_new();
     QObject *listified;
     const char *s;
 
+    if (help) {
+        *help = false;
+    }
+
     s = params;
     while (*s) {
-        s = keyval_parse_one(qdict, s, implied_key, errp);
+        s = keyval_parse_one(qdict, s, implied_key, help, errp);
         if (!s) {
             qobject_unref(qdict);
             return NULL;
-- 
2.25.4

[PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help()

[Kevin Wolf]

This creates separate helper functions for printing a list of user
creatable object types and for printing a list of properties of a given
type. This allows using these parts without having a QemuOpts.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qom/object_interfaces.c | 90 ++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 38 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index e8e1523960..3fd1da157e 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -214,54 +214,68 @@ char *object_property_help(const char *name, const char *type,
     return g_string_free(str, false);
 }
 
-bool user_creatable_print_help(const char *type, QemuOpts *opts)
+static void user_creatable_print_types(void)
+{
+    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);
+}
+
+static bool user_creatable_print_type_properites(const char *type)
 {
     ObjectClass *klass;
+    ObjectPropertyIterator iter;
+    ObjectProperty *prop;
+    GPtrArray *array;
+    int i;
 
-    if (is_help_option(type)) {
-        GSList *l, *list;
+    klass = object_class_by_name(type);
+    if (!klass) {
+        return false;
+    }
 
-        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));
+    array = g_ptr_array_new();
+    object_class_property_iter_init(&iter, klass);
+    while ((prop = object_property_iter_next(&iter))) {
+        if (!prop->set) {
+            continue;
         }
-        g_slist_free(list);
-        return true;
+
+        g_ptr_array_add(array,
+                        object_property_help(prop->name, prop->type,
+                                             prop->defval, prop->description));
     }
+    g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
+    if (array->len > 0) {
+        printf("%s options:\n", type);
+    } else {
+        printf("There are no options for %s.\n", type);
+    }
+    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);
+    return true;
+}
 
-    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))) {
-            if (!prop->set) {
-                continue;
-            }
-
-            g_ptr_array_add(array,
-                            object_property_help(prop->name, prop->type,
-                                                 prop->defval, prop->description));
-        }
-        g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
-        if (array->len > 0) {
-            printf("%s options:\n", type);
-        } else {
-            printf("There are no options for %s.\n", type);
-        }
-        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);
+bool user_creatable_print_help(const char *type, QemuOpts *opts)
+{
+    if (is_help_option(type)) {
+        user_creatable_print_types();
         return true;
     }
 
+    if (qemu_opt_has_help_opt(opts)) {
+        return user_creatable_print_type_properites(type);
+    }
+
     return false;
 }
 
-- 
2.25.4

[PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict()

[Kevin Wolf]

This adds a function that, given a QDict of non-help options, prints
help for user creatable objects.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qom/object_interfaces.h | 9 +++++++++
 qom/object_interfaces.c         | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index f118fb516b..53b114b11a 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -161,6 +161,15 @@ int user_creatable_add_opts_foreach(void *opaque,
  */
 bool user_creatable_print_help(const char *type, QemuOpts *opts);
 
+/**
+ * user_creatable_print_help_from_qdict:
+ * @args: options to create
+ *
+ * Prints help considering the other options given in @args (if "qom-type" is
+ * given and valid, print properties for the type, otherwise print valid types)
+ */
+void user_creatable_print_help_from_qdict(QDict *args);
+
 /**
  * user_creatable_del:
  * @id: the unique ID for the object
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 3fd1da157e..ed896fe764 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -279,6 +279,15 @@ bool user_creatable_print_help(const char *type, QemuOpts *opts)
     return false;
 }
 
+void user_creatable_print_help_from_qdict(QDict *args)
+{
+    const char *type = qdict_get_try_str(args, "qom-type");
+
+    if (!type || !user_creatable_print_type_properites(type)) {
+        user_creatable_print_types();
+    }
+}
+
 bool user_creatable_del(const char *id, Error **errp)
 {
     Object *container;
-- 
2.25.4

[PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser

[Kevin Wolf]

The command line parser for --object parses the input twice: Once into
QemuOpts just for detecting help options, and then again into a QDict
using the keyval parser for actually creating the object.

Now that the keyval parser can also detect help options, we can simplify
this and remove the QemuOpts part.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index bb9cb740f0..7cbdbf0b23 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -264,21 +264,14 @@ static void process_options(int argc, char *argv[])
             }
         case OPTION_OBJECT:
             {
-                QemuOpts *opts;
-                const char *type;
                 QDict *args;
+                bool help;
 
-                /* FIXME The keyval parser rejects 'help' arguments, so we must
-                 * unconditionall try QemuOpts first. */
-                opts = qemu_opts_parse(&qemu_object_opts,
-                                       optarg, true, &error_fatal);
-                type = qemu_opt_get(opts, "qom-type");
-                if (type && user_creatable_print_help(type, opts)) {
+                args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
+                if (help) {
+                    user_creatable_print_help_from_qdict(args);
                     exit(EXIT_SUCCESS);
                 }
-                qemu_opts_del(opts);
-
-                args = keyval_parse(optarg, "qom-type", NULL, &error_fatal);
                 user_creatable_add_dict(args, true, &error_fatal);
                 qobject_unref(args);
                 break;
-- 
2.25.4

Re: [PATCH v2 1/4] keyval: Parse help options

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 5957 bytes --]

On 9/30/20 7:45 AM, Kevin Wolf wrote:
> This adds a new parameter 'help' to keyval_parse() that enables parsing
> of help options. If NULL is passed, the function behaves the same as
> before. But if a bool pointer is given, it contains the information
> whether an option "help" without value was given (which would otherwise
> either result in an error or be interpreted as the value for an implied
> key).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +
> +    /* "help" is only a help option if it has no value */
> +    qdict = keyval_parse("help=on", NULL, &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "help"), ==, "on");
> +    g_assert_false(help);
> +    qobject_unref(qdict);
> +
> +    /* Double comma after "help" in an implied key is not a help option */
> +    qdict = keyval_parse("help,,abc", "implied", &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_false(help);
> +    qobject_unref(qdict);

Worth checking qdict_get_try_str(qdict, "implied") for "help,abc"?

> +
> +    /* Without implied key and without value, it's an error */
> +    qdict = keyval_parse("help,,abc", NULL, &help, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* "help" as the only option */
> +    qdict = keyval_parse("help", NULL, &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> +    g_assert_true(help);
> +    qobject_unref(qdict);
> +
> +    /* "help" as the first part of the key */
> +    qdict = keyval_parse("help.abc", NULL, &help, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

Worth checking qdict_get_try_str(qdict, "help.abc") for "on"? (at least,
that's my guess as what happened)

> +
> +    /* "help" as the last part of the key */
> +    qdict = keyval_parse("abc.help", NULL, &help, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

[1]

> +
> +    /* "help" is not a value for the implied key if &help is given */
> +    qdict = keyval_parse("help", "implied", &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> +    g_assert_true(help);
> +    qobject_unref(qdict);

Worth checking that the qdict does not contain "implied"?  Perhaps by
checking qdict_size() == 0?

> +
> +    /* "help" is a value for the implied key when passing NULL for help */
> +    qdict = keyval_parse("help", "implied", NULL, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help");
> +    qobject_unref(qdict);
> +
> +    /* "help.abc" is a value for the implied key */
> +    qdict = keyval_parse("help.abc", "implied", &help, &err);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help.abc");
> +    g_assert_false(help);
> +    qobject_unref(qdict);
> +
> +    /* "abc.help" is a value for the implied key */
> +    qdict = keyval_parse("abc.help", "implied", &help, &err);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "abc.help");
> +    g_assert_false(help);
> +    qobject_unref(qdict);
> +
> +    /* "help" as the last part of the key */
> +    qdict = keyval_parse("abc.help", NULL, &help, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);

duplicates [1]

> +
> +    /* Other keys before and after help are still parsed normally */
> +    qdict = keyval_parse("number=42,help,foo=bar", NULL, &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "number"), ==, "42");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
> +    g_assert_true(help);
> +    qobject_unref(qdict);
> +
> +    /* ...even with an implied key */
> +    qdict = keyval_parse("val,help,foo=bar", "implied", &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val");
> +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
> +    g_assert_true(help);
> +    qobject_unref(qdict);
>  }
>  

Overall a nice set of additions.  You could tweak it further, but I'm no
longer seeing a hole like last time.

> +++ b/util/keyval.c
> @@ -166,7 +166,7 @@ static QObject *keyval_parse_put(QDict *cur,
>   * On failure, return NULL.
>   */
>  static const char *keyval_parse_one(QDict *qdict, const char *params,
> -                                    const char *implied_key,
> +                                    const char *implied_key, bool *help,
>                                      Error **errp)
>  {
>      const char *key, *key_end, *s, *end;
> @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>      if (key == implied_key) {
>          assert(!*s);
>          s = params;
> +    } else if (*s == '=') {
> +        s++;
>      } else {
> -        if (*s != '=') {
> +        if (help && !strncmp(key, "help", s - key)) {

Should this use is_help_option() to also accept "?", or are we okay
demanding exactly "help"?

> +            *help = true;
> +            if (*s) {
> +                s++;
> +            }
> +            return s;
> +        } else {
>              error_setg(errp, "Expected '=' after parameter '%.*s'",
>                         (int)(s - key), key);
>              return NULL;
>          }
> -        s++;
>      }

Assuming you can touch up the testsuite before the pull request, and
assuming we don't care about "?",

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

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


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

Re: [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help()

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 700 bytes --]

On 9/30/20 7:45 AM, Kevin Wolf wrote:
> This creates separate helper functions for printing a list of user
> creatable object types and for printing a list of properties of a given
> type. This allows using these parts without having a QemuOpts.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qom/object_interfaces.c | 90 ++++++++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 38 deletions(-)

I don't see any changes from v1, so my R-b from there could have been
applied.

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

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


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

Re: [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict()

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 536 bytes --]

On 9/30/20 7:45 AM, Kevin Wolf wrote:
> This adds a function that, given a QDict of non-help options, prints
> help for user creatable objects.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qom/object_interfaces.h | 9 +++++++++
>  qom/object_interfaces.c         | 9 +++++++++
>  2 files changed, 18 insertions(+)

As with v1,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 731 bytes --]

On 9/30/20 7:45 AM, Kevin Wolf wrote:
> The command line parser for --object parses the input twice: Once into
> QemuOpts just for detecting help options, and then again into a QDict
> using the keyval parser for actually creating the object.
> 
> Now that the keyval parser can also detect help options, we can simplify
> this and remove the QemuOpts part.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  storage-daemon/qemu-storage-daemon.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)

As with v1,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [PATCH v2 1/4] keyval: Parse help options

[Kevin Wolf]

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

Am 30.09.2020 um 15:35 hat Eric Blake geschrieben:
> On 9/30/20 7:45 AM, Kevin Wolf wrote:
> > This adds a new parameter 'help' to keyval_parse() that enables parsing
> > of help options. If NULL is passed, the function behaves the same as
> > before. But if a bool pointer is given, it contains the information
> > whether an option "help" without value was given (which would otherwise
> > either result in an error or be interpreted as the value for an implied
> > key).
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> > +
> > +    /* "help" is only a help option if it has no value */
> > +    qdict = keyval_parse("help=on", NULL, &help, &error_abort);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "help"), ==, "on");
> > +    g_assert_false(help);
> > +    qobject_unref(qdict);
> > +
> > +    /* Double comma after "help" in an implied key is not a help option */
> > +    qdict = keyval_parse("help,,abc", "implied", &help, &error_abort);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> > +    g_assert_false(help);
> > +    qobject_unref(qdict);
> 
> Worth checking qdict_get_try_str(qdict, "implied") for "help,abc"?

Yes, this makes sense.

> > +
> > +    /* Without implied key and without value, it's an error */
> > +    qdict = keyval_parse("help,,abc", NULL, &help, &err);
> > +    error_free_or_abort(&err);
> > +    g_assert(!qdict);
> > +
> > +    /* "help" as the only option */
> > +    qdict = keyval_parse("help", NULL, &help, &error_abort);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> > +    g_assert_true(help);
> > +    qobject_unref(qdict);
> > +
> > +    /* "help" as the first part of the key */
> > +    qdict = keyval_parse("help.abc", NULL, &help, &err);
> > +    error_free_or_abort(&err);
> > +    g_assert(!qdict);
> 
> Worth checking qdict_get_try_str(qdict, "help.abc") for "on"? (at least,
> that's my guess as what happened)

The keyval parser doesn't support boolean options like this (I think
this is intentional because it would be ambiguous with implied keys).

This is an error case, and I don't think the QDict has defined content
then. But if anything, we would want to check that it's empty.

> > +
> > +    /* "help" as the last part of the key */
> > +    qdict = keyval_parse("abc.help", NULL, &help, &err);
> > +    error_free_or_abort(&err);
> > +    g_assert(!qdict);
> 
> [1]
> 
> > +
> > +    /* "help" is not a value for the implied key if &help is given */
> > +    qdict = keyval_parse("help", "implied", &help, &error_abort);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> > +    g_assert_true(help);
> > +    qobject_unref(qdict);
> 
> Worth checking that the qdict does not contain "implied"?  Perhaps by
> checking qdict_size() == 0?

Already there, the first line after keyval_parse(). :-)

> > +
> > +    /* "help" is a value for the implied key when passing NULL for help */
> > +    qdict = keyval_parse("help", "implied", NULL, &error_abort);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help");
> > +    qobject_unref(qdict);
> > +
> > +    /* "help.abc" is a value for the implied key */
> > +    qdict = keyval_parse("help.abc", "implied", &help, &err);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help.abc");
> > +    g_assert_false(help);
> > +    qobject_unref(qdict);
> > +
> > +    /* "abc.help" is a value for the implied key */
> > +    qdict = keyval_parse("abc.help", "implied", &help, &err);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 1);
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "abc.help");
> > +    g_assert_false(help);
> > +    qobject_unref(qdict);
> > +
> > +    /* "help" as the last part of the key */
> > +    qdict = keyval_parse("abc.help", NULL, &help, &err);
> > +    error_free_or_abort(&err);
> > +    g_assert(!qdict);
> 
> duplicates [1]

So we can be extra sure!

Somehow I suspect I wanted to test another case here, but I'm not sure
which one. Maybe the same with an implied key? Or I can just remove it.

> > +
> > +    /* Other keys before and after help are still parsed normally */
> > +    qdict = keyval_parse("number=42,help,foo=bar", NULL, &help, &error_abort);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "number"), ==, "42");
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
> > +    g_assert_true(help);
> > +    qobject_unref(qdict);
> > +
> > +    /* ...even with an implied key */
> > +    qdict = keyval_parse("val,help,foo=bar", "implied", &help, &error_abort);
> > +    g_assert_cmpuint(qdict_size(qdict), ==, 2);
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val");
> > +    g_assert_cmpstr(qdict_get_try_str(qdict, "foo"), ==, "bar");
> > +    g_assert_true(help);
> > +    qobject_unref(qdict);
> >  }
> >  
> 
> Overall a nice set of additions.  You could tweak it further, but I'm no
> longer seeing a hole like last time.
> 
> > +++ b/util/keyval.c
> > @@ -166,7 +166,7 @@ static QObject *keyval_parse_put(QDict *cur,
> >   * On failure, return NULL.
> >   */
> >  static const char *keyval_parse_one(QDict *qdict, const char *params,
> > -                                    const char *implied_key,
> > +                                    const char *implied_key, bool *help,
> >                                      Error **errp)
> >  {
> >      const char *key, *key_end, *s, *end;
> > @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
> >      if (key == implied_key) {
> >          assert(!*s);
> >          s = params;
> > +    } else if (*s == '=') {
> > +        s++;
> >      } else {
> > -        if (*s != '=') {
> > +        if (help && !strncmp(key, "help", s - key)) {
> 
> Should this use is_help_option() to also accept "?", or are we okay
> demanding exactly "help"?

The comment for is_help_option() calls "?" deprecated, so I think we
don't want to enable it in a new parser.

> > +            *help = true;
> > +            if (*s) {
> > +                s++;
> > +            }
> > +            return s;
> > +        } else {
> >              error_setg(errp, "Expected '=' after parameter '%.*s'",
> >                         (int)(s - key), key);
> >              return NULL;
> >          }
> > -        s++;
> >      }
> 
> Assuming you can touch up the testsuite before the pull request, and
> assuming we don't care about "?",
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] keyval: Parse help options

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 30.09.2020 um 15:35 hat Eric Blake geschrieben:
>> On 9/30/20 7:45 AM, Kevin Wolf wrote:
>> > This adds a new parameter 'help' to keyval_parse() that enables parsing
>> > of help options. If NULL is passed, the function behaves the same as
>> > before. But if a bool pointer is given, it contains the information
>> > whether an option "help" without value was given (which would otherwise
>> > either result in an error or be interpreted as the value for an implied
>> > key).
>> > 
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
[...]
>> > +++ b/util/keyval.c
>> > @@ -166,7 +166,7 @@ static QObject *keyval_parse_put(QDict *cur,
>> >   * On failure, return NULL.
>> >   */
>> >  static const char *keyval_parse_one(QDict *qdict, const char *params,
>> > -                                    const char *implied_key,
>> > +                                    const char *implied_key, bool *help,
>> >                                      Error **errp)
>> >  {
>> >      const char *key, *key_end, *s, *end;
>> > @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>> >      if (key == implied_key) {
>> >          assert(!*s);
>> >          s = params;
>> > +    } else if (*s == '=') {
>> > +        s++;
>> >      } else {
>> > -        if (*s != '=') {
>> > +        if (help && !strncmp(key, "help", s - key)) {
>> 
>> Should this use is_help_option() to also accept "?", or are we okay
>> demanding exactly "help"?
>
> The comment for is_help_option() calls "?" deprecated, so I think we
> don't want to enable it in a new parser.

Valid point.

But do we really want comparisons for "help" inline everywhere we want
to check for non-deprecated help requests?  Would a common helper
function be better?

On the deprecation of "?": the comment is more than eight years old
(commit c8057f951d6).  We didn't have a deprecation process back then,
but we did purge '?' from the documentation.  Can we finally drop it?
I'm going to ask that in a new thread.

[...]

Re: [PATCH v2 1/4] keyval: Parse help options

[Kevin Wolf]

Am 01.10.2020 um 12:34 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 30.09.2020 um 15:35 hat Eric Blake geschrieben:
> >> On 9/30/20 7:45 AM, Kevin Wolf wrote:
> >> > This adds a new parameter 'help' to keyval_parse() that enables parsing
> >> > of help options. If NULL is passed, the function behaves the same as
> >> > before. But if a bool pointer is given, it contains the information
> >> > whether an option "help" without value was given (which would otherwise
> >> > either result in an error or be interpreted as the value for an implied
> >> > key).
> >> > 
> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> [...]
> >> > +++ b/util/keyval.c
> >> > @@ -166,7 +166,7 @@ static QObject *keyval_parse_put(QDict *cur,
> >> >   * On failure, return NULL.
> >> >   */
> >> >  static const char *keyval_parse_one(QDict *qdict, const char *params,
> >> > -                                    const char *implied_key,
> >> > +                                    const char *implied_key, bool *help,
> >> >                                      Error **errp)
> >> >  {
> >> >      const char *key, *key_end, *s, *end;
> >> > @@ -238,13 +238,20 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
> >> >      if (key == implied_key) {
> >> >          assert(!*s);
> >> >          s = params;
> >> > +    } else if (*s == '=') {
> >> > +        s++;
> >> >      } else {
> >> > -        if (*s != '=') {
> >> > +        if (help && !strncmp(key, "help", s - key)) {
> >> 
> >> Should this use is_help_option() to also accept "?", or are we okay
> >> demanding exactly "help"?
> >
> > The comment for is_help_option() calls "?" deprecated, so I think we
> > don't want to enable it in a new parser.
> 
> Valid point.
> 
> But do we really want comparisons for "help" inline everywhere we want
> to check for non-deprecated help requests?  Would a common helper
> function be better?

How many more parsers are we going to add? I thought we intended the
keyval parser to be the one canoncial place in the long term?

If you prefer, I can make this a second static inline function in
include/qemu/help_option.h (that would have to work both as strcmp and
as strncmp), but I don't see a second user that would make it a "common"
helper.

> On the deprecation of "?": the comment is more than eight years old
> (commit c8057f951d6).  We didn't have a deprecation process back then,
> but we did purge '?' from the documentation.  Can we finally drop it?
> I'm going to ask that in a new thread.

I guess we can.

Kevin

Re: [PATCH v2 1/4] keyval: Parse help options

[Markus Armbruster]

Fried brain today, I have to go through this real slow.  I apologize in
advance for being denser and more error-prone than usual.

Kevin Wolf <kwolf@redhat.com> writes:

> This adds a new parameter 'help' to keyval_parse() that enables parsing
> of help options. If NULL is passed, the function behaves the same as
> before. But if a bool pointer is given, it contains the information
> whether an option "help" without value was given (which would otherwise
> either result in an error or be interpreted as the value for an implied
> key).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

You change the parser, but neglected to update the grammar in the big
file comment.

I made the mistake of attempting to review the parser change without
fixing up the grammar first, and got lost in the weeds.  In part because
today is not my day, but also because doing parsers without a grammar
never works out well for me.

Grammar from the big file comment:

 * KEY=VALUE,... syntax:
 *
 *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
 *   key-val      = key '=' val
 *   key          = key-fragment { '.' key-fragment }
 *   key-fragment = / [^=,.]* /
 *   val          = { / [^,]* / | ',,' }

and

 * Additional syntax for use with an implied key:
 *
 *   key-vals-ik  = val-no-key [ ',' key-vals ]
 *   val-no-key   = / [^=,]* /
 *
 * where no-key is syntactic sugar for implied-key=val-no-key.

According to the commit message, we want to recognize "help" in place of
key-val and val-no-key, but only for callers that opt in.

This is more complex than recognizing it in place of just key-vals.  The
commit message doesn't say why the extra complexity is wanted.  Peeking
ahead in the series, I can see it's for supporting

    -object TYPE,help

in addition to

    -object help

I see.

Making help support opt-in complicates things.  Is there a genuine use
for not supporting help?  Or is just to keep the users that don't
support help yet (but should) working without change?  Mind, I'm not
asking you to make them work, I'm only asking whether you can think of a
genuine case where help should not work.

What are the existing users that don't support help?  Let's see... many
in tests/test-keyval.c (ignore), and qobject_input_visitor_new_str().
That one's used for qemu-system-FOO -audiodev, -display, -blockdev, and
for qemu-storage-daemon --blockdev, --export, --monitor, --nbd-server.

Attempting to get help for them fails like this:

    $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev help
    qemu-storage-daemon: Invalid parameter 'help'
    $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev file,help
    qemu-storage-daemon: Expected '=' after parameter 'help'

I believe making them fail like

    qemu-storage-daemon: no help available

would actually be an improvement.

If we get rid of the case "help is not supported", the grammar update
isn't too bad, something like

 *   key-val      = 'help' | key '=' val

and

 *   key-vals-ik  = 'help' | val-no-key [ ',' key-vals ]

Done for today, hope my brain unfries itself overnight.

[...]

Re: [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This creates separate helper functions for printing a list of user
> creatable object types and for printing a list of properties of a given
> type. This allows using these parts without having a QemuOpts.

Does the last sentence allude to a future patch?  If yes, I suggest to
phrase it as "This will allow ..."

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qom/object_interfaces.c | 90 ++++++++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index e8e1523960..3fd1da157e 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -214,54 +214,68 @@ char *object_property_help(const char *name, const char *type,
>      return g_string_free(str, false);
>  }
>  
> -bool user_creatable_print_help(const char *type, QemuOpts *opts)
> +static void user_creatable_print_types(void)
> +{
> +    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);
> +}
> +
> +static bool user_creatable_print_type_properites(const char *type)
>  {
>      ObjectClass *klass;
> +    ObjectPropertyIterator iter;
> +    ObjectProperty *prop;
> +    GPtrArray *array;
> +    int i;
>  
> -    if (is_help_option(type)) {
> -        GSList *l, *list;
> +    klass = object_class_by_name(type);
> +    if (!klass) {
> +        return false;
> +    }
>  
> -        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));
> +    array = g_ptr_array_new();
> +    object_class_property_iter_init(&iter, klass);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        if (!prop->set) {
> +            continue;
>          }
> -        g_slist_free(list);
> -        return true;
> +
> +        g_ptr_array_add(array,
> +                        object_property_help(prop->name, prop->type,
> +                                             prop->defval, prop->description));
>      }
> +    g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
> +    if (array->len > 0) {
> +        printf("%s options:\n", type);
> +    } else {
> +        printf("There are no options for %s.\n", type);
> +    }
> +    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);
> +    return true;
> +}
>  
> -    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))) {
> -            if (!prop->set) {
> -                continue;
> -            }
> -
> -            g_ptr_array_add(array,
> -                            object_property_help(prop->name, prop->type,
> -                                                 prop->defval, prop->description));
> -        }
> -        g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
> -        if (array->len > 0) {
> -            printf("%s options:\n", type);
> -        } else {
> -            printf("There are no options for %s.\n", type);
> -        }
> -        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);
> +bool user_creatable_print_help(const char *type, QemuOpts *opts)
> +{
> +    if (is_help_option(type)) {
> +        user_creatable_print_types();
>          return true;
>      }
>  
> +    if (qemu_opt_has_help_opt(opts)) {
> +        return user_creatable_print_type_properites(type);
> +    }
> +
>      return false;
>  }

I'd make user_creatable_print_types() return true for summetry with
user_creatable_print_type_properites(), but that's a matter of taste.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This adds a function that, given a QDict of non-help options, prints
> help for user creatable objects.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qom/object_interfaces.h | 9 +++++++++
>  qom/object_interfaces.c         | 9 +++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index f118fb516b..53b114b11a 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -161,6 +161,15 @@ int user_creatable_add_opts_foreach(void *opaque,
>   */
>  bool user_creatable_print_help(const char *type, QemuOpts *opts);
>  
> +/**
> + * user_creatable_print_help_from_qdict:
> + * @args: options to create
> + *
> + * Prints help considering the other options given in @args (if "qom-type" is
> + * given and valid, print properties for the type, otherwise print valid types)
> + */
> +void user_creatable_print_help_from_qdict(QDict *args);
> +
>  /**
>   * user_creatable_del:
>   * @id: the unique ID for the object
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 3fd1da157e..ed896fe764 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -279,6 +279,15 @@ bool user_creatable_print_help(const char *type, QemuOpts *opts)
>      return false;
>  }
>  
> +void user_creatable_print_help_from_qdict(QDict *args)
> +{
> +    const char *type = qdict_get_try_str(args, "qom-type");
> +
> +    if (!type || !user_creatable_print_type_properites(type)) {
> +        user_creatable_print_types();
> +    }

Existing user_creatable_print_help():

1. "qom-type=help,..." and its sugared forms, in particular "help"

   List QOM types and succeed.

2. "qom-type=T,help,..." 

2a. If T names a QOM type

    List T's properties and succeed.

2b. If T does not name a QOM type

    Fail.  Callers typically interpret this as "no help requested",
    proceed, then choke on invalid qom-type=T.

New user_creatable_print_help() treats case 2b like case 1.

Intentional?

> +}
> +
>  bool user_creatable_del(const char *id, Error **errp)
>  {
>      Object *container;

Re: [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> The command line parser for --object parses the input twice: Once into
> QemuOpts just for detecting help options, and then again into a QDict
> using the keyval parser for actually creating the object.
>
> Now that the keyval parser can also detect help options, we can simplify
> this and remove the QemuOpts part.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  storage-daemon/qemu-storage-daemon.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
> index bb9cb740f0..7cbdbf0b23 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -264,21 +264,14 @@ static void process_options(int argc, char *argv[])
>              }
>          case OPTION_OBJECT:
>              {
> -                QemuOpts *opts;
> -                const char *type;
>                  QDict *args;
> +                bool help;
>  
> -                /* FIXME The keyval parser rejects 'help' arguments, so we must
> -                 * unconditionall try QemuOpts first. */
> -                opts = qemu_opts_parse(&qemu_object_opts,
> -                                       optarg, true, &error_fatal);
> -                type = qemu_opt_get(opts, "qom-type");
> -                if (type && user_creatable_print_help(type, opts)) {
> +                args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
> +                if (help) {
> +                    user_creatable_print_help_from_qdict(args);
>                      exit(EXIT_SUCCESS);
>                  }
> -                qemu_opts_del(opts);
> -
> -                args = keyval_parse(optarg, "qom-type", NULL, &error_fatal);
>                  user_creatable_add_dict(args, true, &error_fatal);
>                  qobject_unref(args);
>                  break;

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> This adds a function that, given a QDict of non-help options, prints
>> help for user creatable objects.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  include/qom/object_interfaces.h | 9 +++++++++
>>  qom/object_interfaces.c         | 9 +++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
>> index f118fb516b..53b114b11a 100644
>> --- a/include/qom/object_interfaces.h
>> +++ b/include/qom/object_interfaces.h
>> @@ -161,6 +161,15 @@ int user_creatable_add_opts_foreach(void *opaque,
>>   */
>>  bool user_creatable_print_help(const char *type, QemuOpts *opts);
>>  
>> +/**
>> + * user_creatable_print_help_from_qdict:
>> + * @args: options to create
>> + *
>> + * Prints help considering the other options given in @args (if "qom-type" is
>> + * given and valid, print properties for the type, otherwise print valid types)
>> + */
>> +void user_creatable_print_help_from_qdict(QDict *args);
>> +
>>  /**
>>   * user_creatable_del:
>>   * @id: the unique ID for the object
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index 3fd1da157e..ed896fe764 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -279,6 +279,15 @@ bool user_creatable_print_help(const char *type, QemuOpts *opts)
>>      return false;
>>  }
>>  
>> +void user_creatable_print_help_from_qdict(QDict *args)
>> +{
>> +    const char *type = qdict_get_try_str(args, "qom-type");
>> +
>> +    if (!type || !user_creatable_print_type_properites(type)) {
>> +        user_creatable_print_types();
>> +    }
>
> Existing user_creatable_print_help():
>
> 1. "qom-type=help,..." and its sugared forms, in particular "help"
>
>    List QOM types and succeed.
>
> 2. "qom-type=T,help,..." 
>
> 2a. If T names a QOM type
>
>     List T's properties and succeed.
>
> 2b. If T does not name a QOM type
>
>     Fail.  Callers typically interpret this as "no help requested",
>     proceed, then choke on invalid qom-type=T.

And of course

  3. Else

     No help requested; fail.

> New user_creatable_print_help() treats case 2b like case 1.
>
> Intentional?

The next patch relies on this, so I figure the answer is yes.

The difference to user_creatable_print_help() is subtle.  Perhaps the
contract should point it out explicitly.  Up to you.

By the way, the contract of user_creatable_print_help() is inaccurate:

 * Prints help if requested in @opts.
 *
 * Returns: true if @opts contained a help option and help was printed, false
 * if no help option was found.

One, it prints help when either @type or @opts request it.

Two, "if no help option was found" is misleading: case 2b.

Not this patch's problem.
>
>> +}
>> +
>>  bool user_creatable_del(const char *id, Error **errp)
>>  {
>>      Object *container;

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 1/4] keyval: Parse help options

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

[...]
> Making help support opt-in complicates things.  Is there a genuine use
> for not supporting help?  Or is just to keep the users that don't
> support help yet (but should) working without change?  Mind, I'm not
> asking you to make them work, I'm only asking whether you can think of a
> genuine case where help should not work.
>
> What are the existing users that don't support help?  Let's see... many
> in tests/test-keyval.c (ignore), and qobject_input_visitor_new_str().
> That one's used for qemu-system-FOO -audiodev, -display, -blockdev, and
> for qemu-storage-daemon --blockdev, --export, --monitor, --nbd-server.
>
> Attempting to get help for them fails like this:
>
>     $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev help
>     qemu-storage-daemon: Invalid parameter 'help'
>     $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev file,help
>     qemu-storage-daemon: Expected '=' after parameter 'help'
>
> I believe making them fail like
>
>     qemu-storage-daemon: no help available
>
> would actually be an improvement.

Potential issue: if an option's implied key may have the value "help",
then option argument "help" can be either parsed as "give me help", or
as "implied-key=help".

None of the existing options have this issue:

* audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver,
   "help" is not among its values

* display: union DisplayOptions, implied key "type" is enum
   DisplayType, "help" is not among its values

* blockdev: union BlockdevOptions, implied key "driver is enum
   BlockdevDriver, "help" is not among its values

* export: union BlockExport, implied key "type" is enum BlockExportType,
  "help" is not among its values

* monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode,
  "help" is not among its values

* nbd-server: struct NbdServerOptions, no implied key.

We're good.

What's the risk of not staying good?

The implied keys above are all QAPI enums.

The only existing QAPI enum with a value 'help' is QKeyCode.

The only existing C enum with a name that ends in _HELP is the one
defining Mac keycodes in hw/input/adb-keys.h.

For completeness, I double-checked non of the existing occurences of
string "help" are used as values of option parameters.

We'll almost certainly stay good.

What if we manage to mess this up against all odds?

In my opinion, consistency in getting help is more important than
consistency within a badly designed option: option argument "help"
should give you help even when that means you can't omit the implied key
when its value is "help".

[...]