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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]