Re: [PATCH v4 4/7] keyval: Parse help options

[Eric Blake <eblake@redhat.com>]

On 10/11/20 2:35 AM, Markus Armbruster wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> 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
> output 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.
> 

> +
> +    /* "help" by itself, without implied key */
> +    qdict = keyval_parse("help", NULL, &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> +    g_assert(help);
> +    qobject_unref(qdict);
> +
> +    /* "help" by itself, with implied key */
> +    qdict = keyval_parse("help", "implied", &help, &error_abort);
> +    g_assert_cmpuint(qdict_size(qdict), ==, 0);
> +    g_assert(help);
> +    qobject_unref(qdict);
> +
> +    /* "help" when no help is available, without implied key */
> +    qdict = keyval_parse("help", NULL, NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* "help" when no help is available, with implied key */
> +    qdict = keyval_parse("help", "implied", NULL, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* Key "help" */
> +    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(!help);
> +    qobject_unref(qdict);
> +
> +    /* "help" followed by crap, without implied key */
> +    qdict = keyval_parse("help.abc", NULL, &help, &err);
> +    error_free_or_abort(&err);
> +    g_assert(!qdict);
> +
> +    /* "help" followed by crap, with 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(!help);
> +    qobject_unref(qdict);
> +
> +    /* "help" with other stuff, without implied key */
> +    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(help);
> +    qobject_unref(qdict);
> +
> +    /* "help" with other stuff, with 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(help);
> +    qobject_unref(qdict);

Is it worth checking that "helper" with implied key is a value, not help?

> +++ b/util/keyval.c
> @@ -14,10 +14,11 @@
>    * KEY=VALUE,... syntax:
>    *
>    *   key-vals     = [ key-val { ',' key-val } [ ',' ] ]
> - *   key-val      = key '=' val
> + *   key-val      = key '=' val | help
>    *   key          = key-fragment { '.' key-fragment }
>    *   key-fragment = / [^=,.]+ /
>    *   val          = { / [^,]+ / | ',,' }
> + *   help         = 'help | '?'

Missing '

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

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