Vladimir Sementsov-Ogievskiy <vsement...@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 <arm...@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 <vsement...@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;
>> }
>> }
