On 3/12/21 9:58 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <phi...@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 <kw...@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@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.
