Re: [Qemu-devel] [PATCH] Use error_is_set() only when necessary

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 01/30/2014 07:07 AM, Markus Armbruster wrote:
>> error_is_set(&var) is the same as var != NULL, but it takes
>> whole-program analysis to figure that out.  Unnecessarily hard for
>> optimizers, static checkers, and human readers.  Dumb it down to
>> obvious.
>> 
>> Gets rid of several dozen Coverity false positives.
>> 
>> Note that the obvious form is already used in many places.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>>  37 files changed, 156 insertions(+), 156 deletions(-)
>
> Good diffstat - shows it should be fairly mechanical.
>
>> @@ -1399,7 +1399,7 @@ fail:
>>      QDECREF(bs->options);
>>      QDECREF(options);
>>      bs->options = NULL;
>> -    if (error_is_set(&local_err)) {
>> +    if (local_err) {
>>          error_propagate(errp, local_err);
>>      }
>>      return ret;
>
> Is it worth a further cleanup on instances like this?  That is,
> error_propagate(errp, NULL) is a safe no-op, so we can avoid the 'if
> (local_err)' conditional.  But that should not be in this patch (keep
> the mechanical changes easy).

I like your suggestion, and I agree it should be a separate patch.

>> +++ b/block/snapshot.c
>> @@ -345,7 +345,7 @@ int 
>> bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
>>          ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, &local_err);
>>      }
>>  
>> -    if (error_is_set(&local_err)) {
>> +    if (local_err) {
>>          error_propagate(errp, local_err);
>>      }
>
> Another example that can be simplified.
>
>> +++ b/tests/test-qmp-input-strict.c
>> @@ -92,7 +92,7 @@ static void test_validate_struct(TestInputVisitorData 
>> *data,
>>      v = validate_test_init(data, "{ 'integer': -42, 'boolean':
>> true, 'string': 'foo' }");
>>  
>>      visit_type_TestStruct(v, &p, NULL, &errp);
>> -    g_assert(!error_is_set(&errp));
>> +    g_assert(!errp);
>
> This (and other places in test files) chould use
> visit_type_TestStruct(v, &p, NULL, &error_abort) and ditch local errp.
> But that's a separate patch as well.
>
> Reviewed-by: Eric Blake <address@hidden>

I like this one, too.  Thanks!