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

[Kevin Wolf]

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

signature.asc
Description: PGP signature