* [PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser
@ 2020-09-30 12:45 Kevin Wolf
2020-09-30 12:45 ` [PATCH v2 1/4] keyval: Parse help options Kevin Wolf
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Kevin Wolf @ 2020-09-30 12:45 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz
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
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v2 1/4] keyval: Parse help options 2020-09-30 12:45 [PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf @ 2020-09-30 12:45 ` Kevin Wolf 2020-09-30 13:35 ` Eric Blake 2020-10-01 15:46 ` Markus Armbruster 2020-09-30 12:45 ` [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf ` (2 subsequent siblings) 3 siblings, 2 replies; 18+ messages in thread From: Kevin Wolf @ 2020-09-30 12:45 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] keyval: Parse help options 2020-09-30 12:45 ` [PATCH v2 1/4] keyval: Parse help options Kevin Wolf @ 2020-09-30 13:35 ` Eric Blake 2020-09-30 15:10 ` Kevin Wolf 2020-10-01 15:46 ` Markus Armbruster 1 sibling, 1 reply; 18+ messages in thread From: Eric Blake @ 2020-09-30 13:35 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel, armbru, mreitz [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] keyval: Parse help options 2020-09-30 13:35 ` Eric Blake @ 2020-09-30 15:10 ` Kevin Wolf 2020-10-01 10:34 ` Markus Armbruster 0 siblings, 1 reply; 18+ messages in thread From: Kevin Wolf @ 2020-09-30 15:10 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel, armbru, qemu-block, mreitz [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] keyval: Parse help options 2020-09-30 15:10 ` Kevin Wolf @ 2020-10-01 10:34 ` Markus Armbruster 2020-10-01 11:33 ` Kevin Wolf 0 siblings, 1 reply; 18+ messages in thread From: Markus Armbruster @ 2020-10-01 10:34 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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. [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] keyval: Parse help options 2020-10-01 10:34 ` Markus Armbruster @ 2020-10-01 11:33 ` Kevin Wolf 0 siblings, 0 replies; 18+ messages in thread From: Kevin Wolf @ 2020-10-01 11:33 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, qemu-block, mreitz 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] keyval: Parse help options 2020-09-30 12:45 ` [PATCH v2 1/4] keyval: Parse help options Kevin Wolf 2020-09-30 13:35 ` Eric Blake @ 2020-10-01 15:46 ` Markus Armbruster 2020-10-05 12:08 ` Markus Armbruster 1 sibling, 1 reply; 18+ messages in thread From: Markus Armbruster @ 2020-10-01 15:46 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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. [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] keyval: Parse help options 2020-10-01 15:46 ` Markus Armbruster @ 2020-10-05 12:08 ` Markus Armbruster 0 siblings, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-05 12:08 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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". [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() 2020-09-30 12:45 [PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-09-30 12:45 ` [PATCH v2 1/4] keyval: Parse help options Kevin Wolf @ 2020-09-30 12:45 ` Kevin Wolf 2020-09-30 13:46 ` Eric Blake 2020-10-02 12:13 ` Markus Armbruster 2020-09-30 12:45 ` [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf 2020-09-30 12:45 ` [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 3 siblings, 2 replies; 18+ messages in thread From: Kevin Wolf @ 2020-09-30 12:45 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() 2020-09-30 12:45 ` [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf @ 2020-09-30 13:46 ` Eric Blake 2020-10-02 12:13 ` Markus Armbruster 1 sibling, 0 replies; 18+ messages in thread From: Eric Blake @ 2020-09-30 13:46 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel, armbru, mreitz [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() 2020-09-30 12:45 ` [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf 2020-09-30 13:46 ` Eric Blake @ 2020-10-02 12:13 ` Markus Armbruster 1 sibling, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-02 12:13 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() 2020-09-30 12:45 [PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-09-30 12:45 ` [PATCH v2 1/4] keyval: Parse help options Kevin Wolf 2020-09-30 12:45 ` [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf @ 2020-09-30 12:45 ` Kevin Wolf 2020-09-30 13:48 ` Eric Blake 2020-10-02 12:25 ` Markus Armbruster 2020-09-30 12:45 ` [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 3 siblings, 2 replies; 18+ messages in thread From: Kevin Wolf @ 2020-09-30 12:45 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() 2020-09-30 12:45 ` [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf @ 2020-09-30 13:48 ` Eric Blake 2020-10-02 12:25 ` Markus Armbruster 1 sibling, 0 replies; 18+ messages in thread From: Eric Blake @ 2020-09-30 13:48 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel, armbru, mreitz [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() 2020-09-30 12:45 ` [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf 2020-09-30 13:48 ` Eric Blake @ 2020-10-02 12:25 ` Markus Armbruster 2020-10-02 12:36 ` Markus Armbruster 1 sibling, 1 reply; 18+ messages in thread From: Markus Armbruster @ 2020-10-02 12:25 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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; ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() 2020-10-02 12:25 ` Markus Armbruster @ 2020-10-02 12:36 ` Markus Armbruster 0 siblings, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-02 12:36 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser 2020-09-30 12:45 [PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf ` (2 preceding siblings ...) 2020-09-30 12:45 ` [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf @ 2020-09-30 12:45 ` Kevin Wolf 2020-09-30 13:49 ` Eric Blake 2020-10-02 12:26 ` Markus Armbruster 3 siblings, 2 replies; 18+ messages in thread From: Kevin Wolf @ 2020-09-30 12:45 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser 2020-09-30 12:45 ` [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf @ 2020-09-30 13:49 ` Eric Blake 2020-10-02 12:26 ` Markus Armbruster 1 sibling, 0 replies; 18+ messages in thread From: Eric Blake @ 2020-09-30 13:49 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel, armbru, mreitz [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser 2020-09-30 12:45 ` [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-09-30 13:49 ` Eric Blake @ 2020-10-02 12:26 ` Markus Armbruster 1 sibling, 0 replies; 18+ messages in thread From: Markus Armbruster @ 2020-10-02 12:26 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-10-05 12:09 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 12:45 [PATCH v2 0/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-09-30 12:45 ` [PATCH v2 1/4] keyval: Parse help options Kevin Wolf 2020-09-30 13:35 ` Eric Blake 2020-09-30 15:10 ` Kevin Wolf 2020-10-01 10:34 ` Markus Armbruster 2020-10-01 11:33 ` Kevin Wolf 2020-10-01 15:46 ` Markus Armbruster 2020-10-05 12:08 ` Markus Armbruster 2020-09-30 12:45 ` [PATCH v2 2/4] qom: Factor out helpers from user_creatable_print_help() Kevin Wolf 2020-09-30 13:46 ` Eric Blake 2020-10-02 12:13 ` Markus Armbruster 2020-09-30 12:45 ` [PATCH v2 3/4] qom: Add user_creatable_print_help_from_qdict() Kevin Wolf 2020-09-30 13:48 ` Eric Blake 2020-10-02 12:25 ` Markus Armbruster 2020-10-02 12:36 ` Markus Armbruster 2020-09-30 12:45 ` [PATCH v2 4/4] qemu-storage-daemon: Remove QemuOpts from --object parser Kevin Wolf 2020-09-30 13:49 ` Eric Blake 2020-10-02 12:26 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.