Re: [PATCH 05/25] keyval: simplify keyval_parse_one

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Now that the key is NULL terminated, we can remove some of the contortions
> that were done to operate on possibly '='-terminated strings in
> keyval_parse_one.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Alright, I'm now ready to discuss the third argument: nicer code.

I think there is improvement, and I think it comes mostly from working
on a copy.  Could be done without the syntax change.

I feel further improvement is possible, but that's no reason to delay
this series.


Second argument: better error messages.  We discussed that in review of
a prior post, but you've since improved your error reporting further, so
let's have another look.  I'm testing with

    $ qemu-storage-daemon --nbd $ARG

because that one doesn't have an implied key, which permits slightly
simpler $ARG.

* Empty key

  --nbd ,

    master:       Invalid parameter ''
    your patch:   Expected parameter before ','

    Improvement.

  --nbd key=val,=,fob=

    master:       Invalid parameter ''
    your patch:   Expected parameter before '=,fob='

    Improvement.

* Empty key fragment

  --nbd key..=

    master:       Invalid parameter 'key..'
    your patch:   same

    Could use some love.  Not your patch's fault.

  --nbd .key=

    master:       Invalid parameter '..key'
    your patch:   same

    Likweise.

  If I omit the '=', your patch's message changes to

                  No implicit parameter name for value 'key..'

  I consider that worse than before, because it's talking about
  something outside the user's control (lack of an implict parameter
  name) where it should instead tell the user what needs fixing in the
  input.

* Missing value

  --nbd key

    master:       Expected '=' after parameter 'key'
    your patch:   No implicit parameter name for value 'key'

  Same criticism as above.

* Invalid key fragment

  --nbd _=

    master:       Invalid parameter '_'
    your patch:   same

  --nbd key.1a.b=

    master:       Invalid parameter 'key.1a.b'
    your patch:   same

    Could perhaps use some love.  Not your patch's fault.

  --ndb anti,,social,,key=

    master:       Expected '=' after parameter 'anti'
    your patch:   Invalid parameter 'anti,social,key'

    The new error message shows the *unescaped* string.  Okay.

Your patch also adds an "Expected parameter at end of string" error.
Can you tell me how to trigger it?

I think there is improvement, and I think it could also be done without
the syntax change.

There also is one regression: "No implicit parameter name..." is no
good.  Looks fixable to me.

I feel further improvement is possible, but that's no reason to delay
this series.


Now let me put the three arguments together.

Nicer code and better error reporting could be had with or without the
syntax change.  With is a bit easier, because we already have your
patch.

The syntax change is a choice we can make.  I'm reluctant to mess with
the syntax, but if you want the change, I'm not going to block it.

Hmm, bartering opportunity...  May I have your support for me
eliminating anti-social device names in exchange?  ;)

I believe your grammar is ambiguous.  Your code seems to pick the sane
alternative.  If I'm wrong, you need to enlighten me.  If I'm right, you
need to fix your grammar.

> ---
>  util/keyval.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/util/keyval.c b/util/keyval.c
> index eb9b9c55ec..e7f708cd1e 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -170,11 +170,10 @@ static QObject *keyval_parse_put(QDict *cur,
>   *
>   * On return:
>   * - either NUL or the separator (comma or equal sign) is returned.
> - * - the length of the string is stored in @len.
>   * - @start is advanced to either the NUL or the first character past the
>   *   separator.
>   */
> -static char keyval_fetch_string(char **start, size_t *len, bool key)
> +static char keyval_fetch_string(char **start, bool key)
>  {
>      char sep;
>      char *p, *unescaped;
> @@ -197,7 +196,6 @@ static char keyval_fetch_string(char **start, size_t *len, bool key)
>      }
>  
>      *unescaped = 0;
> -    *len = unescaped - *start;
>      *start = p;
>      return sep;
>  }
> @@ -219,7 +217,7 @@ static char *keyval_parse_one(QDict *qdict, char *params,
>                                const char *implied_key, bool *help,
>                                Error **errp)
>  {
> -    const char *key, *key_end, *s, *end;
> +    const char *key, *s, *end;
>      const char *val = NULL;
>      char sep;
>      size_t len;
> @@ -229,8 +227,8 @@ static char *keyval_parse_one(QDict *qdict, char *params,
>      QObject *next;
>  
>      key = params;
> -    sep = keyval_fetch_string(&params, &len, true);
> -    if (!len) {
> +    sep = keyval_fetch_string(&params, true);
> +    if (!*key) {
>          if (sep) {
>              error_setg(errp, "Expected parameter before '%c%s'", sep, params);
>          } else {
> @@ -247,13 +245,11 @@ static char *keyval_parse_one(QDict *qdict, char *params,
>              /* Desugar implied key */
>              val = key;
>              key = implied_key;
> -            len = strlen(implied_key);
>          } else {
>              error_setg(errp, "No implicit parameter name for value '%s'", key);
>              return NULL;
>          }
>      }
> -    key_end = key + len;
>  
>      /*
>       * Loop over key fragments: @s points to current fragment, it
> @@ -269,24 +265,21 @@ static char *keyval_parse_one(QDict *qdict, char *params,
>              ret = parse_qapi_name(s, false);
>              len = ret < 0 ? 0 : ret;
>          }
> -        assert(s + len <= key_end);
> -        if (!len || (s + len < key_end && s[len] != '.')) {
> +        if (!len || (s[len] != '\0' && s[len] != '.')) {
>              assert(key != implied_key);
> -            error_setg(errp, "Invalid parameter '%.*s'",
> -                       (int)(key_end - key), key);
> +            error_setg(errp, "Invalid parameter '%s'", key);
>              return NULL;
>          }
>          if (len >= sizeof(key_in_cur)) {
>              assert(key != implied_key);
>              error_setg(errp, "Parameter%s '%.*s' is too long",
> -                       s != key || s + len != key_end ? " fragment" : "",
> +                       s != key || s[len] == '.' ? " fragment" : "",
>                         (int)len, s);
>              return NULL;
>          }
>  
>          if (s != key) {
> -            next = keyval_parse_put(cur, key_in_cur, NULL,
> -                                    key, s - 1, errp);
> +            next = keyval_parse_put(cur, key_in_cur, NULL, key, s - 1, errp);

Unrelated line join.

>              if (!next) {
>                  return NULL;
>              }
> @@ -301,9 +294,9 @@ static char *keyval_parse_one(QDict *qdict, char *params,
>  
>      if (key != implied_key) {
>          val = params;
> -        keyval_fetch_string(&params, &len, false);
> +        keyval_fetch_string(&params, false);
>      }
> -    if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
> +    if (!keyval_parse_put(cur, key_in_cur, val, key, s - 1, errp)) {
>          return NULL;
>      }
>      return params;