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 <kw...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
