Re: [Qemu-devel] [PATCH for-1.4] tests/test-string-input-visitor: Handle errors provoked by fuzz test

[Peter Maydell]

On 2 February 2013 21:37, Andreas Färber <address@hidden> 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 <address@hidden>
>> ---
>>  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.

-- PMM