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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 6/24/20 11:43 AM, 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:
>
> This seems to be a recurring cleanup (witness commit 06592d7e,
> c0e90679, 6b62d961).  In fact, shouldn't you just update that script
> with your enhancements here, and then run it directly, instead of
> embedding your tweaks in the commit message?

I didn't remember scripts/coccinelle/remove_local_err.cocci.

remove_local_err.cocci transforms

    fun(..., &err);
    error_propagate(errp, err);

to

    fun(..., errp);

when the context permits that, using a conservative approximation of
"context permits".  In other words, the script is sound.

My script matches more, but is unsound: it *can* mess up things.  For
instance

    Error err = NULL;

    foo(1, 2, 3, &err);
    error_propagate(errp, err);
    return !err;

would become

    Error err = NULL;

    foo(1, 2, 3, errp);
    return !err;

Oops.  See "manually double-check" below.  For a one-shot cleanup,
manual checking is much, much easier for me than making the Coccinelle
script sound without sacrificing (too much) matching power.

>
>>
>>      @@
>>      identifier fun, err, errp;
>>      expression list args;
>>      @@
>>      -    fun(args, &err);
>>      +    fun(args, errp);
>>           ... when != err
>>               when strict
>>      -    error_propagate(errp, err);
>
> What does the 'when strict' accomplish?  The existing coccinelle
> script uses 'when != errp', which may be enough to address...

Magic!  Without it, I get

   diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
   index 8310c77ec0..2355999171 100644
   --- a/hw/vfio/pci.c
   +++ b/hw/vfio/pci.c
   @@ -1511,16 +1511,14 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int 
pos, Error **errp)
        ret = msix_init(&vdev->pdev, vdev->msix->entries,
                        vdev->bars[vdev->msix->table_bar].mr,
                        vdev->msix->table_bar, vdev->msix->table_offset,
   -                    vdev->bars[vdev->msix->pba_bar].mr,
   -                    vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
   -                    &err);
   +                    vdev->bars[vdev->msix->pba_bar].mr, vdev->msix->pba_bar,
   +                    vdev->msix->pba_offset, pos, errp);
        if (ret < 0) {
            if (ret == -ENOTSUP) {
WTF!?! --->     warn_report_err(err);
                return 0;
            }

   -        error_propagate(errp, err);
            return ret;
        }

Since Coccinelle's documentation is of no help whatsoever (again), I
went looking for possibly applicable ideas in existing scripts.  I found
"when strict" in the kernel's scripts/coccinelle/ directory.  I couldn't
find it in Coccinelle docs.  So I gave it a try, and here we are.

Did I mention Coccinelle is terrible?

>> 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.
>
> ...the control-flow failures you hit?

This one I can actually explain, I think.  Consider this code snippet
from net.c:

 1      visit_type_Netdev(v, NULL, &object, &err);

 2      if (!err) {
 3          ret = net_client_init1(object, is_netdev, &err);
 4      }

 5      qapi_free_Netdev(object);

 6  out:
 7      error_propagate(errp, err);

There are two overlapping matches of rule 1: either line 1 and 7, or
line 3 and 7.  Which one is right?  Coccinelle can't decide, and
implodes:

    rule starting on line 10: node 96: error_propagate(...)[1,2,41,42] in 
net_client_init reachable by inconsistent control-flow paths

>>
>> 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>
>> ---
>
>>   22 files changed, 31 insertions(+), 73 deletions(-)
>
> At any rate, it's small enough to ensure all the changes remaining are
> still valid.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!