[Note cc: Luiz] Peter Maydell <peter.mayd...@linaro.org> writes:
> On 2 February 2013 21:37, Andreas Färber <afaer...@suse.de> wrote: >> Am 02.02.2013 22:19, schrieb Peter Maydell: >>> It's OK and expected for visitors to return errors when presented with >>> the fuzz test's random data. This means the test harness needs to >>> handle them; check for and free any error after each visitor call, >>> and only free the string returned by visit_type_str if visit_type_str >>> succeeded. >>> >>> This fixes a problem where this test failed the MacOSX malloc() >>> consistency checks and might segfault on other platforms [due >>> to calling free() on an uninitialized pointer variable]. >>> >>> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> >>> --- >>> tests/test-string-input-visitor.c | 23 ++++++++++++++++++++++- >>> 1 file changed, 22 insertions(+), 1 deletion(-) >>> >>> diff --git a/tests/test-string-input-visitor.c >>> b/tests/test-string-input-visitor.c >>> index f6b0093..793b334 100644 >>> --- a/tests/test-string-input-visitor.c >>> +++ b/tests/test-string-input-visitor.c >>> @@ -194,20 +194,41 @@ static void test_visitor_in_fuzz(TestInputVisitorData >>> *data, >>> >>> v = visitor_input_test_init(data, buf); >>> visit_type_int(v, &ires, NULL, &errp); >>> + if (error_is_set(&errp)) { >>> + error_free(errp); >>> + errp = NULL; >>> + } >> >> It seems to me the naming is bad here: errp appears to be an Error*, not >> an Error**. It would be nice to fix this within the function touched. > > "Error *errp" is blessed by docs/writing-qmp-commands.txt (and > git grep 'Error \*errp' has 80 examples in the tree). I think > if I were writing this code I'd probably agree with you about the > naming, but I'm not and I don't particularly feel like changing > names somebody else has been consistent about in this source file > in the course of fixing a bug. > >> Since it is an Error*, I think it was said that we should not use >> error_is_set() but err != NULL (or if you prefer, just err). >> error_is_set() is intended for **errp arguments that may be NULL. > > Calling error_is_set(&some_local_err_ptr) is also in the > examples in the docs. If not doing that is the recommendation > there should be a doc comment in error.h about that. I'd find this clearer: v = visitor_input_test_init(data, buf); visit_type_int(v, &ires, NULL, &errp); error_free(errp); errp = NULL; Makes it blatantly obvious that the error is freed and the pointer reset no matter what.