Re: [Qemu-devel] [PATCH v15 21/23] qapi: Fix string input visitor handling of invalid list

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> As shown in the previous commit, the string input visitor was
> treating bogus input as an empty list rather than an error.
> Fix parse_str() to set errp, then the callers to exit early if
> an error was reported.  Also, make sure the error message
> for visit_type_uint64() gracefully handles a NULL 'name' when
> called from the top level or a list context.
>
> Meanwhile, fix the testsuite to use the generated
> qapi_free_int16List() instead of rolling our own, and to
> validate the fixed behavior, while at the same time documenting
> one more change that we'd like to make in a later patch (a
> failed visit_start_list should guarantee a NULL pointer,
> regardless of what things were on input).
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v15: new patch
> ---
>  qapi/string-input-visitor.c       | 19 +++++++++++++------
>  tests/test-string-input-visitor.c | 12 +++++-------
>  2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 797973a..ad150dc 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -44,7 +44,7 @@ static void free_range(void *range, void *dummy)
>      g_free(range);
>  }
>
> -static void parse_str(StringInputVisitor *siv, Error **errp)
> +static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>  {
>      char *str = (char *) siv->string;
>      long long start, end;
> @@ -52,7 +52,7 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
>      char *endptr;
>
>      if (siv->ranges) {
> -        return;
> +        return 0;
>      }
>
>      do {
> @@ -117,11 +117,14 @@ static void parse_str(StringInputVisitor *siv, Error 
> **errp)
>          }
>      } while (str);
>
> -    return;
> +    return 0;
>  error:
>      g_list_foreach(siv->ranges, free_range, NULL);
>      g_list_free(siv->ranges);
>      siv->ranges = NULL;
> +    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> +               "an int64 value or range");
> +    return -1;
>  }

You make the function return success/failure so that you can do

    if (parse_str(..., errp) < 0) {
        ...
    }

instead of the cumbersome and less readable

    Error *err = NULL;

    parse_str(..., &err);
    if (err) {
        error_propagate(errp, err);
        ...
    }

For what it's worth, GLib recommends doing this practice with GError,
except with true / false instead of 0 / -1 (thanks to Marc-André for
pointing this out to me).  I got a private branch where I'm
investigating adopting this convention across QEMU.

>
>  static void
> @@ -129,7 +132,9 @@ start_list(Visitor *v, const char *name, Error **errp)
>  {
>      StringInputVisitor *siv = to_siv(v);
>
> -    parse_str(siv, errp);
> +    if (parse_str(siv, name, errp) < 0) {
> +        return;
> +    }
>
>      siv->cur_range = g_list_first(siv->ranges);
>      if (siv->cur_range) {

Is parse_str()'s error message "Parameter '%s' expects an int64 value or
range" appropriate here?

> @@ -195,7 +200,9 @@ static void parse_type_int64(Visitor *v, const char 
> *name, int64_t *obj,
>          return;
>      }
>
> -    parse_str(siv, errp);
> +    if (parse_str(siv, name, errp) < 0) {
> +        return;
> +    }
>
>      if (!siv->ranges) {
>          goto error;

parse_str()'s error message is appropriate here.  It duplicates the one
visible below, though.  Makes me wonder why parse_str() returns an Error
object.  I guess it'll makes sense once we improve it to return more
specific errors.  Anyway, let's not worry about it now.

> @@ -222,7 +229,7 @@ static void parse_type_int64(Visitor *v, const char 
> *name, int64_t *obj,
>      return;
>
>  error:
> -    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>                 "an int64 value or range");

Separate bug, separate patch?

>  }
>
> diff --git a/tests/test-string-input-visitor.c 
> b/tests/test-string-input-visitor.c
> index 8114908..f99824d 100644
> --- a/tests/test-string-input-visitor.c
> +++ b/tests/test-string-input-visitor.c
> @@ -92,19 +92,17 @@ static void test_visitor_in_intList(TestInputVisitorData 
> *data,
>      }
>      g_assert(!tmp);
>
> -    tmp = res;
> -    while (tmp) {
> -        res = res->next;
> -        g_free(tmp);
> -        tmp = res;
> -    }
> +    qapi_free_int16List(res);
>
>      visitor_input_teardown(data, unused);
>
>      v = visitor_input_test_init(data, "not an int list");
>
> +    /* FIXME: res should be NULL on failure, regardless of starting value */
> +    res = NULL;
>      visit_type_int16List(v, NULL, &res, &err);
> -    /* FIXME fix the visitor, then error_free_or_abort(&err) here */
> +    error_free_or_abort(&err);
> +    g_assert(!res);
>  }
>
>  static void test_visitor_in_bool(TestInputVisitorData *data,