* [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal @ 2021-03-11 19:27 Philippe Mathieu-Daudé 2021-03-12 8:58 ` Markus Armbruster 0 siblings, 1 reply; 3+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-11 19:27 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Philippe Mathieu-Daudé, Markus Armbruster Calls passing &error_abort or &error_fatal don't return. Any code after such use is dubious. Add a coccinelle patch to detect such pattern. 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 */ + ... + } +) 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> -- 2.26.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal 2021-03-11 19:27 [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal Philippe Mathieu-Daudé @ 2021-03-12 8:58 ` Markus Armbruster 2021-03-12 9:57 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 3+ messages in thread From: Markus Armbruster @ 2021-03-12 8:58 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel 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> ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal 2021-03-12 8:58 ` Markus Armbruster @ 2021-03-12 9:57 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 3+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-12 9:57 UTC (permalink / raw) To: Markus Armbruster; +Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-12 10:00 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-03-11 19:27 [PATCH] scripts/coccinelle: Catch dubious code after &error_abort/&error_fatal Philippe Mathieu-Daudé 2021-03-12 8:58 ` Markus Armbruster 2021-03-12 9:57 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).