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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> 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?

Case /* "help" followed by crap, with implied key */ covers this,
doesn't it?

>> +++ 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 '

Kevin fixed it up.

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

Thanks!