Re: [PATCH 06/46] error: Avoid error_propagate() when error is not used here

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 24.06.2020 19:43, Markus Armbruster wrote:
>> When all we do with an Error we receive into a local variable is
>> propagating to somewhere else, we can just as well receive it there
>> right away.  Coccinelle script:
>>
>>      @@
>>      identifier fun, err, errp;
>>      expression list args;
>>      @@
>>      -    fun(args, &err);
>>      +    fun(args, errp);
>>           ... when != err
>>               when strict
>>      -    error_propagate(errp, err);
>>
>>      @@
>>      identifier fun, err, errp;
>>      expression list args;
>>      expression ret;
>>      @@
>>      -    ret = fun(args, &err);
>>      +    ret = fun(args, errp);
>>           ... when != err
>>               when strict
>>      -    error_propagate(errp, err);
>>
>>      @@
>>      identifier fun, err, errp;
>>      expression list args;
>>      expression ret;
>>      @@
>>      -    ret = fun(args, &err);
>>      +    ret = fun(args, errp);
>>           ... when != err
>>               when strict
>>           if (
>>      (
>>               ret
>>      |
>>               !ret
>>      |
>>               ret == 0
>>      |
>>               ret != 0
>>      |
>>               ret < 0
>>      |
>>               ret != NULL
>>      |
>>               ret == NULL
>>      )
>>              )
>>           {
>>               ... when != err
>>                   when strict
>>      -        error_propagate(errp, err);
>>               ...
>>           }
>>
>>      @@
>>      identifier fun, err, errp;
>>      expression list args;
>>      @@
>>           if (
>>      (
>>      -        fun(args, &err)
>>      +        fun(args, errp)
>>      |
>>      -        !fun(args, &err)
>>      +        !fun(args, errp)
>>      |
>>      -        fun(args, &err) == 0
>>      +        fun(args, errp) == 0
>>      |
>>      -        fun(args, &err) != 0
>>      +        fun(args, errp) != 0
>>      |
>>      -        fun(args, &err) < 0
>>      +        fun(args, errp) < 0
>>      |
>>      -        fun(args, &err) == NULL
>>      +        fun(args, errp) == NULL
>>      |
>>      -        fun(args, &err) != NULL
>>      +        fun(args, errp) != NULL
>>      )
>>              )
>>           {
>>               ... when != err;
>>                   when strict
>>      -        error_propagate(errp, err);
>>               ...
>>           }
>>
>> The first two rules are prone to fail with "error_propagate(...)
>> ... reachable by inconsistent control-flow paths".  Script without
>> them re-run where that happens.
>>
>> Manually double-check @err is not used afterwards.  Dereferencing it
>> would be use after free, but checking whether it's null would be
>> legitimate.  One such change to qbus_realize() backed out.
>>
>> Suboptimal line breaks tweaked manually.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> [..]
>
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index 8d6156578f..6705220380 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -316,9 +316,8 @@ int ics_set_kvm_state(ICSState *ics, Error **errp)
>>               continue;
>>           }
>
> local_err becomes unused in this function, we should drop it

Will fix.

> with this fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks!

>
>>   -        ret = ics_set_kvm_state_one(ics, i, &local_err);
>> +        ret = ics_set_kvm_state_one(ics, i, errp);
>>           if (ret < 0) {
>> -            error_propagate(errp, local_err);
>>               return ret;
>>           }
>>       }