qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal
Date: Fri, 12 Mar 2021 09:58:56 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)

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.

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...

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.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1e15dab8cd4..db6596eb06d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2368,6 +2368,7 @@ F: scripts/coccinelle/error_propagate_null.cocci
>  F: scripts/coccinelle/remove_local_err.cocci
>  F: scripts/coccinelle/use-error_fatal.cocci
>  F: scripts/coccinelle/errp-guard.cocci
> +F: scripts/coccinelle/use-after-abort-fatal-errp.cocci
>  
>  GDB stub
>  M: Alex Bennée <alex.bennee@linaro.org>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]