Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> > > --- > 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,