Re: [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/12/21 9:58 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> Calls passing &error_abort or &error_fatal don't return.
> 
> Correction: they *can* return.  What they can't is return failure.
> 
>> Any code after such use is dubious. Add a coccinelle patch
>> to detect such pattern.
> 
> To be precise: any failure path from such a call is dead code.
> 
>> Inspired-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  .../use-after-abort-fatal-errp.cocci          | 33 +++++++++++++++++++
>>  MAINTAINERS                                   |  1 +
>>  2 files changed, 34 insertions(+)
>>  create mode 100644 scripts/coccinelle/use-after-abort-fatal-errp.cocci
>>
>> diff --git a/scripts/coccinelle/use-after-abort-fatal-errp.cocci b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
>> new file mode 100644
>> index 00000000000..ead9de5826a
>> --- /dev/null
>> +++ b/scripts/coccinelle/use-after-abort-fatal-errp.cocci
>> @@ -0,0 +1,33 @@
>> +/* Find dubious code use after error_abort/error_fatal
>> + *
>> + * Inspired by this patch:
>> + * https://www.mail-archive.com/qemu-devel@nongnu.org/msg789501.html
>> + *
>> + * Copyright (C) 2121 Red Hat, Inc.
>> + *
>> + * Authors:
>> + *  Philippe Mathieu-Daudé
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +@@
>> +identifier func_with_errp;
>> +@@
>> +(
>> + if (func_with_errp(..., &error_fatal)) {
>> +    /* Used for displaying help message */
>> +    ...
>> +    exit(...);
>> +  }
>> +|
>> +*if (func_with_errp(..., &error_fatal)) {
>> +    /* dubious code */
>> +    ...
>> +  }
>> +|
>> +*if (func_with_errp(..., &error_abort)) {
>> +    /* dubious code */
>> +    ...
>> +  }
>> +)
> 
> This assumes func_with_errp() returns a falsy value on failure.
> Functions returning bool and pointers do that by convention, but not
> functions returning (signed) integers:
> 
>  * - Whenever practical, also return a value that indicates success /
>  *   failure.  This can make the error checking more concise, and can
>  *   avoid useless error object creation and destruction.  Note that
>  *   we still have many functions returning void.  We recommend
>  *   • bool-valued functions return true on success / false on failure,
>  *   • pointer-valued functions return non-null / null pointer, and
>  *   • integer-valued functions return non-negative / negative.
> 
> Special case of integer-valued functions: return zero / negative.  Your
> script gets that one backwards, I'm afraid.

Apparently:

hw/ppc/spapr.c
---
@@ -2900,7 +2900,6 @@ static void spapr_machine_init(MachineSt
     }

     /* Graphics */
*    if (spapr_vga_init(phb->bus, &error_fatal)) {
         spapr->has_graphics = true;
         machine->usb |= defaults_enabled() && !machine->usb_disabled;
     }
---

qemu-img.c
---
@@ -589,9 +589,6 @@ static int img_create(int argc, char **a
     }
     optind++;

*    if (qemu_opts_foreach(&qemu_object_opts,
*                          user_creatable_add_opts_foreach,
*                          qemu_img_object_print_help, &error_fatal)) {
         goto fail;
     }
---

> Moreover, I expect the convention to be violated in places.
> 
> I'm converned about the script's rate of false positives.  How many
> reports do you get for the whole tree?  Can you guesstimate the rate of
> false positives?
> 
> Next issue.  We commonly assign the return value to a variable before
> checking it, like this:
> 
>     ret = func_with_errp(..., errp);
>     if (!ret) {
>         ...
>         return some error value;
>     }
>     do something with @ret...

Indeed I couldn't catch that easily.

> 
> I suspect your script can't flag dead error handling there.  False
> negatives.  These merely make the script less useful, whereas false
> positives make it less usable, which is arguably worse.

OK, thanks for your analysis, let's drop this patch.