* [PATCH v3 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser
@ 2020-10-07 16:48 Kevin Wolf
2020-10-07 16:49 ` [PATCH v3 1/4] keyval: Parse help options Kevin Wolf
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Kevin Wolf @ 2020-10-07 16:48 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, armbru, qemu-devel
This replaces the QemuOpts-based help code for --object in the storage
daemon with code based on the keyval parser.
v3:
- Always parse help options, no matter if the caller implements help or
not. If it doesn't, return an error. [Markus]
- Document changes to the keyval parser grammar [Markus]
- Support both 'help' and '?' [Eric]
- Test case fixes [Eric]
- Improved documentation of user_creatable_print_help(_from_qdict)
[Markus]
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/help_option.h | 5 +
include/qemu/option.h | 2 +-
include/qom/object_interfaces.h | 21 ++-
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 | 54 ++++++-
8 files changed, 280 insertions(+), 123 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3 1/4] keyval: Parse help options 2020-10-07 16:48 [PATCH v3 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf @ 2020-10-07 16:49 ` Kevin Wolf 2020-10-07 17:29 ` Eric Blake 2020-10-09 15:10 ` Markus Armbruster 2020-10-07 16:49 ` [PATCH v3 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf ` (2 subsequent siblings) 3 siblings, 2 replies; 9+ messages in thread From: Kevin Wolf @ 2020-10-07 16:49 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, qemu-devel This adds a special meaning for 'help' and '?' as options to the keyval parser. Instead of being an error (because of a missing value) or a value for an implied key, they now request help, which is a new boolean ouput of the parser in addition to the QDict. A new parameter 'p_help' is added to keyval_parse() that contains on return whether help was requested. If NULL is passed, requesting help results in an error and all other cases work like before. Turning previous error cases into help is a compatible extension. The behaviour potentially changes for implied keys: They could previously get 'help' as their value, which is now interpreted as requesting help. This is not a problem in practice because 'help' and '?' are not a valid values for the implied key of any option parsed with keyval_parse(): * audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver, "help" and "?" are not among its values * display: union DisplayOptions, implied key "type" is enum DisplayType, "help" and "?" are not among its values * blockdev: union BlockdevOptions, implied key "driver is enum BlockdevDriver, "help" and "?" are not among its values * export: union BlockExport, implied key "type" is enum BlockExportType, "help" and "?" are not among its values * monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode, "help" and "?" are not among its values * nbd-server: struct NbdServerOptions, no implied key. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qemu/help_option.h | 5 + 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 | 54 ++++++- 6 files changed, 198 insertions(+), 72 deletions(-) diff --git a/include/qemu/help_option.h b/include/qemu/help_option.h index 328d2a89fd..952d628b9d 100644 --- a/include/qemu/help_option.h +++ b/include/qemu/help_option.h @@ -19,4 +19,9 @@ static inline bool is_help_option(const char *s) return !strcmp(s, "?") || !strcmp(s, "help"); } +static inline bool is_help_option_n(const char *s, size_t n) +{ + return !strncmp(s, "?", n) || !strncmp(s, "help", n); +} + #endif 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 1ae1cda481..6f0e0cfb36 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..b1433054de 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_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help,abc"); + 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" is an error when passing NULL for p_help */ + qdict = keyval_parse("help", NULL, NULL, &err); + error_free_or_abort(&err); + g_assert(!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 */ + qdict = keyval_parse("help", "implied", &help, &error_abort); + g_assert_cmpuint(qdict_size(qdict), ==, 0); + g_assert_true(help); + qobject_unref(qdict); + + /* "help" is not a value for the implied key even with NULL for p_help */ + qdict = keyval_parse("help", "implied", NULL, &err); + error_free_or_abort(&err); + g_assert(!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); + + /* 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..5929c9dba2 100644 --- a/util/keyval.c +++ b/util/keyval.c @@ -14,7 +14,7 @@ * KEY=VALUE,... syntax: * * key-vals = [ key-val { ',' key-val } [ ',' ] ] - * key-val = key '=' val + * key-val = 'help' | key '=' val * key = key-fragment { '.' key-fragment } * key-fragment = / [^=,.]* / * val = { / [^,]* / | ',,' } @@ -73,10 +73,14 @@ * * Additional syntax for use with an implied key: * - * key-vals-ik = val-no-key [ ',' key-vals ] + * key-vals-ik = 'help' | val-no-key [ ',' key-vals ] * val-no-key = / [^=,]* / * * where no-key is syntactic sugar for implied-key=val-no-key. + * + * 'help' for key-val or key-vals-ik is not interpreted as part of the + * JSON object, but instead requests help, which is a separate boolean + * output of the parser. */ #include "qemu/osdep.h" @@ -85,6 +89,7 @@ #include "qapi/qmp/qlist.h" #include "qapi/qmp/qstring.h" #include "qemu/cutils.h" +#include "qemu/help_option.h" #include "qemu/option.h" /* @@ -166,7 +171,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 +243,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 (is_help_option_n(key, 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 +272,15 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, qstring_append_chr(val, *s++); } + if (key == implied_key) { + const char *str_val = qstring_get_str(val); + if (is_help_option(str_val)) { + *help = true; + qobject_unref(val); + return s; + } + } + if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { return NULL; } @@ -388,21 +409,32 @@ 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 '='. + * + * An option "help" or "?" without a value isn't added to the + * resulting dictionary, but instead is interpreted as requesting + * help. If @p_help is non-NULL, it is true on return if help was + * requested and false otherwise. All other options are parsed and + * returned normally so that context specific help can be printed. + * + * If @p_help is NULL and help is requested, an error is returned. + * * 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 *p_help, Error **errp) { QDict *qdict = qdict_new(); QObject *listified; const char *s; + bool 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; @@ -410,6 +442,14 @@ QDict *keyval_parse(const char *params, const char *implied_key, implied_key = NULL; } + if (p_help) { + *p_help = help; + } else if (help) { + error_setg(errp, "Help is not available for this option"); + qobject_unref(qdict); + return NULL; + } + listified = keyval_listify(qdict, NULL, errp); if (!listified) { qobject_unref(qdict); -- 2.25.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/4] keyval: Parse help options 2020-10-07 16:49 ` [PATCH v3 1/4] keyval: Parse help options Kevin Wolf @ 2020-10-07 17:29 ` Eric Blake 2020-10-08 8:45 ` Kevin Wolf 2020-10-09 15:10 ` Markus Armbruster 1 sibling, 1 reply; 9+ messages in thread From: Eric Blake @ 2020-10-07 17:29 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: armbru, qemu-devel [-- Attachment #1.1: Type: text/plain, Size: 3417 bytes --] On 10/7/20 11:49 AM, Kevin Wolf wrote: > This adds a special meaning for 'help' and '?' as options to the keyval > parser. Instead of being an error (because of a missing value) or a > value for an implied key, they now request help, which is a new boolean > ouput of the parser in addition to the QDict. output > > A new parameter 'p_help' is added to keyval_parse() that contains on > return whether help was requested. If NULL is passed, requesting help > results in an error and all other cases work like before. > > Turning previous error cases into help is a compatible extension. The > behaviour potentially changes for implied keys: They could previously > get 'help' as their value, which is now interpreted as requesting help. > > This is not a problem in practice because 'help' and '?' are not a valid > values for the implied key of any option parsed with keyval_parse(): > > * audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver, > "help" and "?" are not among its values > > * display: union DisplayOptions, implied key "type" is enum > DisplayType, "help" and "?" are not among its values > > * blockdev: union BlockdevOptions, implied key "driver is enum > BlockdevDriver, "help" and "?" are not among its values > > * export: union BlockExport, implied key "type" is enum BlockExportType, > "help" and "?" are not among its values > > * monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode, missing space > "help" and "?" are not among its values > > * nbd-server: struct NbdServerOptions, no implied key. Including the audit is nice (and thanks to Markus for helping derive the list). > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > +++ b/util/keyval.c > @@ -14,7 +14,7 @@ > * KEY=VALUE,... syntax: > * > * key-vals = [ key-val { ',' key-val } [ ',' ] ] > - * key-val = key '=' val > + * key-val = 'help' | key '=' val Maybe: = 'help' | '?' | key = '=' val > * key = key-fragment { '.' key-fragment } > * key-fragment = / [^=,.]* / > * val = { / [^,]* / | ',,' } > @@ -73,10 +73,14 @@ > * > * Additional syntax for use with an implied key: > * > - * key-vals-ik = val-no-key [ ',' key-vals ] > + * key-vals-ik = 'help' | val-no-key [ ',' key-vals ] and again for '?' here. Actually, this should probably be: key-vals-ik = 'help' [ ',' key-vals ] | '?' [ ',' key-vals ] | val-no-key [ ',' key-vals ] > * val-no-key = / [^=,]* / This is now slightly inaccurate, but I don't know how to concisely express the regex [^=,]* but not '?' or 'help', and the prose covers the ambiguity. > @@ -410,6 +442,14 @@ QDict *keyval_parse(const char *params, const char *implied_key, > implied_key = NULL; > } > > + if (p_help) { > + *p_help = help; > + } else if (help) { > + error_setg(errp, "Help is not available for this option"); > + qobject_unref(qdict); > + return NULL; > + } This part is a definite improvement over v2. I'm assuming Markus can help touch up the comments and spelling errors, so: 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/4] keyval: Parse help options 2020-10-07 17:29 ` Eric Blake @ 2020-10-08 8:45 ` Kevin Wolf 2020-10-08 15:25 ` Markus Armbruster 0 siblings, 1 reply; 9+ messages in thread From: Kevin Wolf @ 2020-10-08 8:45 UTC (permalink / raw) To: Eric Blake; +Cc: armbru, qemu-block, qemu-devel [-- Attachment #1: Type: text/plain, Size: 4288 bytes --] Am 07.10.2020 um 19:29 hat Eric Blake geschrieben: > On 10/7/20 11:49 AM, Kevin Wolf wrote: > > This adds a special meaning for 'help' and '?' as options to the keyval > > parser. Instead of being an error (because of a missing value) or a > > value for an implied key, they now request help, which is a new boolean > > ouput of the parser in addition to the QDict. > > output > > > > > A new parameter 'p_help' is added to keyval_parse() that contains on > > return whether help was requested. If NULL is passed, requesting help > > results in an error and all other cases work like before. > > > > Turning previous error cases into help is a compatible extension. The > > behaviour potentially changes for implied keys: They could previously > > get 'help' as their value, which is now interpreted as requesting help. > > > > This is not a problem in practice because 'help' and '?' are not a valid > > values for the implied key of any option parsed with keyval_parse(): > > > > * audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver, > > "help" and "?" are not among its values > > > > * display: union DisplayOptions, implied key "type" is enum > > DisplayType, "help" and "?" are not among its values > > > > * blockdev: union BlockdevOptions, implied key "driver is enum > > BlockdevDriver, "help" and "?" are not among its values > > > > * export: union BlockExport, implied key "type" is enum BlockExportType, > > "help" and "?" are not among its values > > > > * monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode, > > missing space > > > "help" and "?" are not among its values > > > > * nbd-server: struct NbdServerOptions, no implied key. > > Including the audit is nice (and thanks to Markus for helping derive the > list). > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > > +++ b/util/keyval.c > > @@ -14,7 +14,7 @@ > > * KEY=VALUE,... syntax: > > * > > * key-vals = [ key-val { ',' key-val } [ ',' ] ] > > - * key-val = key '=' val > > + * key-val = 'help' | key '=' val > > Maybe: = 'help' | '?' | key = '=' val > > > * key = key-fragment { '.' key-fragment } > > * key-fragment = / [^=,.]* / > > * val = { / [^,]* / | ',,' } > > @@ -73,10 +73,14 @@ > > * > > * Additional syntax for use with an implied key: > > * > > - * key-vals-ik = val-no-key [ ',' key-vals ] > > + * key-vals-ik = 'help' | val-no-key [ ',' key-vals ] > > and again for '?' here. Ah, true, I should mention '?', too. > Actually, this should probably be: > > key-vals-ik = 'help' [ ',' key-vals ] > | '?' [ ',' key-vals ] > | val-no-key [ ',' key-vals ] I noticed that the grammar was wrong even before my patch (because making use of the implied key is optional), but the right fix wasn't obvious to me, so I decided to just leave that part as it is. Even your version is wrong because it's valid to a value with implied key and help at the same time. Thinking a bit more about it, I feel it should simply be something like: key-vals-ik = val-no-key [ ',' key-vals ] | key-vals And then key-vals automatically covers the help case. > > * val-no-key = / [^=,]* / > > This is now slightly inaccurate, but I don't know how to concisely > express the regex [^=,]* but not '?' or 'help', and the prose covers the > ambiguity. > > > > @@ -410,6 +442,14 @@ QDict *keyval_parse(const char *params, const char *implied_key, > > implied_key = NULL; > > } > > > > + if (p_help) { > > + *p_help = help; > > + } else if (help) { > > + error_setg(errp, "Help is not available for this option"); > > + qobject_unref(qdict); > > + return NULL; > > + } > > This part is a definite improvement over v2. > > I'm assuming Markus can help touch up the comments and spelling errors, so: > > Reviewed-by: Eric Blake <eblake@redhat.com> I assumed that as a qsd series this would go through my own tree anyway, so if all of you agree that you don't want to see the corrected version on the list, I can fix them while applying the series. Kevin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/4] keyval: Parse help options 2020-10-08 8:45 ` Kevin Wolf @ 2020-10-08 15:25 ` Markus Armbruster 0 siblings, 0 replies; 9+ messages in thread From: Markus Armbruster @ 2020-10-08 15:25 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block Kevin Wolf <kwolf@redhat.com> writes: > Am 07.10.2020 um 19:29 hat Eric Blake geschrieben: >> On 10/7/20 11:49 AM, Kevin Wolf wrote: >> > This adds a special meaning for 'help' and '?' as options to the keyval >> > parser. Instead of being an error (because of a missing value) or a >> > value for an implied key, they now request help, which is a new boolean >> > ouput of the parser in addition to the QDict. >> >> output >> >> > >> > A new parameter 'p_help' is added to keyval_parse() that contains on >> > return whether help was requested. If NULL is passed, requesting help >> > results in an error and all other cases work like before. >> > >> > Turning previous error cases into help is a compatible extension. The >> > behaviour potentially changes for implied keys: They could previously >> > get 'help' as their value, which is now interpreted as requesting help. >> > >> > This is not a problem in practice because 'help' and '?' are not a valid >> > values for the implied key of any option parsed with keyval_parse(): >> > >> > * audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver, >> > "help" and "?" are not among its values >> > >> > * display: union DisplayOptions, implied key "type" is enum >> > DisplayType, "help" and "?" are not among its values >> > >> > * blockdev: union BlockdevOptions, implied key "driver is enum >> > BlockdevDriver, "help" and "?" are not among its values >> > >> > * export: union BlockExport, implied key "type" is enum BlockExportType, >> > "help" and "?" are not among its values >> > >> > * monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode, >> >> missing space >> >> > "help" and "?" are not among its values >> > >> > * nbd-server: struct NbdServerOptions, no implied key. >> >> Including the audit is nice (and thanks to Markus for helping derive the >> list). >> >> > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > --- >> >> > +++ b/util/keyval.c >> > @@ -14,7 +14,7 @@ >> > * KEY=VALUE,... syntax: >> > * >> > * key-vals = [ key-val { ',' key-val } [ ',' ] ] >> > - * key-val = key '=' val >> > + * key-val = 'help' | key '=' val >> >> Maybe: = 'help' | '?' | key = '=' val >> >> > * key = key-fragment { '.' key-fragment } >> > * key-fragment = / [^=,.]* / >> > * val = { / [^,]* / | ',,' } >> > @@ -73,10 +73,14 @@ >> > * >> > * Additional syntax for use with an implied key: >> > * >> > - * key-vals-ik = val-no-key [ ',' key-vals ] >> > + * key-vals-ik = 'help' | val-no-key [ ',' key-vals ] >> >> and again for '?' here. > > Ah, true, I should mention '?', too. Let's use a non-terminal help = 'help' | '?' key-val = key '=' val | help >> Actually, this should probably be: >> >> key-vals-ik = 'help' [ ',' key-vals ] >> | '?' [ ',' key-vals ] >> | val-no-key [ ',' key-vals ] > > I noticed that the grammar was wrong even before my patch (because > making use of the implied key is optional), but the right fix wasn't > obvious to me, so I decided to just leave that part as it is. The grammar leaves a small, but important unsaid, or rather said only in the accompanying prose. * Additional syntax for use with an implied key: * * key-vals-ik = val-no-key [ ',' key-vals ] * val-no-key = / [^=,]* / tries to express that with an implied key, the grammar changes from S = key-vals to S = key-vals | key-vals-ik See, "additional syntax". Felt clear enough to me when I wrote it. It is not. > Even your version is wrong because it's valid to a value with implied > key and help at the same time. > > Thinking a bit more about it, I feel it should simply be something like: > > key-vals-ik = val-no-key [ ',' key-vals ] | key-vals > > And then key-vals automatically covers the help case. Unfortunately, this simple grammar is ambigious: "help" can be parsed both as "help" -> help -> key-val -> key-vals -> key-vals-ik and "help" -> val-no-key -> key-vals-ik Ham-fisted fix: val-no-key = / [^-,]* / - 'help' I'm not a fan of the exception operator. Better ideas? I'll have a closer look at the actual patch next. >> > * val-no-key = / [^=,]* / >> >> This is now slightly inaccurate, but I don't know how to concisely >> express the regex [^=,]* but not '?' or 'help', and the prose covers the >> ambiguity. >> >> >> > @@ -410,6 +442,14 @@ QDict *keyval_parse(const char *params, const char *implied_key, >> > implied_key = NULL; >> > } >> > >> > + if (p_help) { >> > + *p_help = help; >> > + } else if (help) { >> > + error_setg(errp, "Help is not available for this option"); >> > + qobject_unref(qdict); >> > + return NULL; >> > + } >> >> This part is a definite improvement over v2. >> >> I'm assuming Markus can help touch up the comments and spelling errors, so: >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > I assumed that as a qsd series this would go through my own tree anyway, > so if all of you agree that you don't want to see the corrected version > on the list, I can fix them while applying the series. The series spans "Command line option argument parsing" (this patch), "QOM" (next two), and qsd (final one). I'm fine with you taking it through your tree. Just noticed: you neglected to cc: the QOM maintainers. Recommend to give them a heads-up. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/4] keyval: Parse help options 2020-10-07 16:49 ` [PATCH v3 1/4] keyval: Parse help options Kevin Wolf 2020-10-07 17:29 ` Eric Blake @ 2020-10-09 15:10 ` Markus Armbruster 1 sibling, 0 replies; 9+ messages in thread From: Markus Armbruster @ 2020-10-09 15:10 UTC (permalink / raw) To: Kevin Wolf; +Cc: armbru, qemu-block, qemu-devel Kevin Wolf <kwolf@redhat.com> writes: > This adds a special meaning for 'help' and '?' as options to the keyval > parser. Instead of being an error (because of a missing value) or a > value for an implied key, they now request help, which is a new boolean > ouput of the parser in addition to the QDict. > > A new parameter 'p_help' is added to keyval_parse() that contains on > return whether help was requested. If NULL is passed, requesting help > results in an error and all other cases work like before. > > Turning previous error cases into help is a compatible extension. The > behaviour potentially changes for implied keys: They could previously > get 'help' as their value, which is now interpreted as requesting help. > > This is not a problem in practice because 'help' and '?' are not a valid > values for the implied key of any option parsed with keyval_parse(): > > * audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver, > "help" and "?" are not among its values > > * display: union DisplayOptions, implied key "type" is enum > DisplayType, "help" and "?" are not among its values > > * blockdev: union BlockdevOptions, implied key "driver is enum > BlockdevDriver, "help" and "?" are not among its values > > * export: union BlockExport, implied key "type" is enum BlockExportType, > "help" and "?" are not among its values > > * monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode, > "help" and "?" are not among its values > > * nbd-server: struct NbdServerOptions, no implied key. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/qemu/help_option.h | 5 + > 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 | 54 ++++++- > 6 files changed, 198 insertions(+), 72 deletions(-) > > diff --git a/include/qemu/help_option.h b/include/qemu/help_option.h > index 328d2a89fd..952d628b9d 100644 > --- a/include/qemu/help_option.h > +++ b/include/qemu/help_option.h > @@ -19,4 +19,9 @@ static inline bool is_help_option(const char *s) > return !strcmp(s, "?") || !strcmp(s, "help"); > } > > +static inline bool is_help_option_n(const char *s, size_t n) > +{ > + return !strncmp(s, "?", n) || !strncmp(s, "help", n); > +} > + > #endif Unlike is_help_option(), this also recognizes prefixes of "help": $ upstream-qemu-storage-daemon --object secret,hel secret options: data=<string> file=<string> format=<QCryptoSecretFormat> iv=<string> keyid=<string> loaded=<bool> $ upstream-qemu-storage-daemon --object hel upstream-qemu-storage-daemon: Parameter 'id' is missing > 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 1ae1cda481..6f0e0cfb36 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..b1433054de 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_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "help,abc"); > + 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); No clearer than g_assert(help) (but it shows off knowledge of GLib). > + qobject_unref(qdict); > + > + /* "help" is an error when passing NULL for p_help */ > + qdict = keyval_parse("help", NULL, NULL, &err); > + error_free_or_abort(&err); > + g_assert(!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 */ > + qdict = keyval_parse("help", "implied", &help, &error_abort); > + g_assert_cmpuint(qdict_size(qdict), ==, 0); > + g_assert_true(help); > + qobject_unref(qdict); > + > + /* "help" is not a value for the implied key even with NULL for p_help */ > + qdict = keyval_parse("help", "implied", NULL, &err); > + error_free_or_abort(&err); > + g_assert(!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); No clearer than g_assert(!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); > + > + /* 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..5929c9dba2 100644 > --- a/util/keyval.c > +++ b/util/keyval.c > @@ -14,7 +14,7 @@ > * KEY=VALUE,... syntax: > * > * key-vals = [ key-val { ',' key-val } [ ',' ] ] > - * key-val = key '=' val > + * key-val = 'help' | key '=' val > * key = key-fragment { '.' key-fragment } > * key-fragment = / [^=,.]* / > * val = { / [^,]* / | ',,' } > @@ -73,10 +73,14 @@ > * > * Additional syntax for use with an implied key: > * > - * key-vals-ik = val-no-key [ ',' key-vals ] > + * key-vals-ik = 'help' | val-no-key [ ',' key-vals ] > * val-no-key = / [^=,]* / > * > * where no-key is syntactic sugar for implied-key=val-no-key. > + * > + * 'help' for key-val or key-vals-ik is not interpreted as part of the > + * JSON object, but instead requests help, which is a separate boolean > + * output of the parser. > */ > > #include "qemu/osdep.h" > @@ -85,6 +89,7 @@ > #include "qapi/qmp/qlist.h" > #include "qapi/qmp/qstring.h" > #include "qemu/cutils.h" > +#include "qemu/help_option.h" > #include "qemu/option.h" > > /* > @@ -166,7 +171,7 @@ static QObject *keyval_parse_put(QDict *cur, > * On failure, return NULL. > */ Contract needs an update. > 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 +243,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 (is_help_option_n(key, 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 +272,15 @@ static const char *keyval_parse_one(QDict *qdict, const char *params, > qstring_append_chr(val, *s++); > } > > + if (key == implied_key) { > + const char *str_val = qstring_get_str(val); > + if (is_help_option(str_val)) { > + *help = true; > + qobject_unref(val); > + return s; > + } > + } > + > if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { > return NULL; > } The patch is minimal, and I believe works except for one corner case (search for Bug: below). But it makes the code even more subtle than it already is. Code before the patch: key = params; len = strcspn(params, "=,"); Both key and val-no-key are / [^=,]* /. The former is followed by '=', the latter is not. This lets us distinguish the two. if (implied_key && len && key[len] != '=') { /* Desugar implied key */ key = implied_key; len = strlen(implied_key); } key_end = key + len; @key, @key_end now bracket what must be the key. [parse from @key to @key_end...] if (key == implied_key) { assert(!*s); s = params; We're parsing val-no-key, and @s points to it. } else { if (*s != '=') { error_setg(errp, "Expected '=' after parameter '%.*s'", (int)(s - key), key); return NULL; } s++; We're parsing key '=' val, and @s points to val. } [parse from @s to unescaped ',' or to '\0'... into @val] if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { return NULL; } return s; The patch adds parsing of help. How it is parsed depends on @implied_key. Without implied_key: key = params; len = strcspn(params, "=,"); if (implied_key && len && key[len] != '=') { [not reached...] } key_end = key + len; @key, @key_end now bracket what must be the key or help. [parse from @key to @key_end...] Bug: parsing help as if it was a key works for 'help' (which parses okay as key), but not for '?' (which doesn't). Reproducer: $ upstream-qemu-storage-daemon --nbd-server help upstream-qemu-storage-daemon: Help is not available for this option $ upstream-qemu-storage-daemon --nbd-server '?' upstream-qemu-storage-daemon: Invalid parameter '?' if (key == implied_key) { [not reached...] } else if (*s == '=') { s++; We're parsing key '=' val, and @s points to val. } else { We're parsing help (if we survived parsing help as key). if (is_help_option_n(key, s - key)) { *help = true; if (*s) { s++; } return s; } else { error_setg(errp, "Expected '=' after parameter '%.*s'", (int)(s - key), key); return NULL; } } [parse from @s to unescaped ',' or to '\0'... into @val] if (key == implied_key) { [not reached...] } if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { return NULL; } return s; With implied_key: key = params; len = strcspn(params, "=,"); if (implied_key && len && key[len] != '=') { /* Desugar implied key */ help takes this branch, too. key = implied_key; len = strlen(implied_key); } key_end = key + len; @key, @key_end now bracket what must be the key. If we're looking at help, it's the implied key, and it'll be ignored, but ... [parse from @key to @key_end...] ... we parse it all the same. No harm in doing that. if (key == implied_key) { assert(!*s); s = params; We're parsing val-no-key | help, and @s points to it. } else if (*s == '=') { s++; We're parsing key '=' val, and @s points to val. } else { Nothing left to parse; we're going to error no matter what. if (is_help_option_n(key, s - key)) { [not reached...] } else { error_setg(errp, "Expected '=' after parameter '%.*s'", (int)(s - key), key); return NULL; } } [parse from @s to unescaped ',' or to '\0'... into @val] if (key == implied_key) { We parsed val-no-key | help. const char *str_val = qstring_get_str(val); if (is_help_option(str_val)) { It's help. *help = true; qobject_unref(val); return s; } } if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) { return NULL; } return s; Your v1 was much simpler. Eric asked "what if the user typed "help,," to get "help," as the value of the implied key?" Good question, but the correct answer is that the value of an implied key cannot contain ',', which means v1 wasn't actually flawed. I'd like to tighten up grammar, code, and tests in a patch that goes first. Then I adjust this one, and post the whole as v4. Okay? [...] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/4] qom: Factor out helpers from user_creatable_print_help() 2020-10-07 16:48 [PATCH v3 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 1/4] keyval: Parse help options Kevin Wolf @ 2020-10-07 16:49 ` Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 3 siblings, 0 replies; 9+ messages in thread From: Kevin Wolf @ 2020-10-07 16:49 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, qemu-devel 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 will allow using these parts without having a QemuOpts. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/4] qom: Add user_creatable_print_help_from_qdict() 2020-10-07 16:48 [PATCH v3 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 1/4] keyval: Parse help options Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf @ 2020-10-07 16:49 ` Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 3 siblings, 0 replies; 9+ messages in thread From: Kevin Wolf @ 2020-10-07 16:49 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, qemu-devel 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> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- include/qom/object_interfaces.h | 21 ++++++++++++++++++--- qom/object_interfaces.c | 9 +++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h index f118fb516b..07d5cc8832 100644 --- a/include/qom/object_interfaces.h +++ b/include/qom/object_interfaces.h @@ -154,13 +154,28 @@ int user_creatable_add_opts_foreach(void *opaque, * @type: the QOM type to be added * @opts: options to create * - * Prints help if requested in @opts. + * Prints help if requested in @type or @opts. Note that if @type is neither + * "help"/"?" nor a valid user creatable type, no help will be printed + * regardless of @opts. * - * Returns: true if @opts contained a help option and help was printed, false - * if no help option was found. + * Returns: true if a help option was found and help was printed, false + * otherwise. */ 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) + * + * In contrast to user_creatable_print_help(), this function can't return that + * no help was requested. It should only be called if we know that help is + * requested and it will always print some help. + */ +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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser 2020-10-07 16:48 [PATCH v3 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf ` (2 preceding siblings ...) 2020-10-07 16:49 ` [PATCH v3 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf @ 2020-10-07 16:49 ` Kevin Wolf 3 siblings, 0 replies; 9+ messages in thread From: Kevin Wolf @ 2020-10-07 16:49 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, armbru, qemu-devel 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> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@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 6f0e0cfb36..e419ba9f19 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-09 15:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-07 16:48 [PATCH v3 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 1/4] keyval: Parse help options Kevin Wolf 2020-10-07 17:29 ` Eric Blake 2020-10-08 8:45 ` Kevin Wolf 2020-10-08 15:25 ` Markus Armbruster 2020-10-09 15:10 ` Markus Armbruster 2020-10-07 16:49 ` [PATCH v3 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf 2020-10-07 16:49 ` [PATCH v3 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).