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

From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [PATCH v2 1/4] keyval: Parse help options
Date: Thu, 1 Oct 2020 13:33:51 +0200	[thread overview]
Message-ID: <20201001113351.GB6673@linux.fritz.box> (raw)
In-Reply-To: <87lfgq1bfx.fsf@dusky.pond.sub.org>

Am 01.10.2020 um 12:34 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > 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>
> [...]
> >> > +++ 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.
> 
> Valid point.
> 
> But do we really want comparisons for "help" inline everywhere we want
> to check for non-deprecated help requests?  Would a common helper
> function be better?

How many more parsers are we going to add? I thought we intended the
keyval parser to be the one canoncial place in the long term?

If you prefer, I can make this a second static inline function in
include/qemu/help_option.h (that would have to work both as strcmp and
as strncmp), but I don't see a second user that would make it a "common"
helper.

> On the deprecation of "?": the comment is more than eight years old
> (commit c8057f951d6).  We didn't have a deprecation process back then,
> but we did purge '?' from the documentation.  Can we finally drop it?
> I'm going to ask that in a new thread.

I guess we can.

Kevin