Eric Blake <ebl...@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 <arm...@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 <ebl...@redhat.com>
Thanks!
