QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Maydell <peter.maydell@linaro.org>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: "Paul Burton" <pburton@wavecomp.com>,
	"Sagar Karandikar" <sagark@eecs.berkeley.edu>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Jason Wang" <jasowang@redhat.com>,
	"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Alistair Francis" <Alistair.Francis@wdc.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Aleksandar Markovic" <aleksandar.qemu.devel@gmail.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Aleksandar Rikalo" <aleksandar.rikalo@rt-rk.com>,
	"Richard Henderson" <rth@twiddle.net>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Alistair Francis" <alistair@alistair23.me>,
	qemu-arm <qemu-arm@nongnu.org>,
	"David Gibson" <david@gibson.dropbear.id.au>,
	"open list:RISC-V" <qemu-riscv@nongnu.org>,
	"Bastian Koppelmann" <kbastian@mail.uni-paderborn.de>,
	"Andrew Baumann" <Andrew.Baumann@microsoft.com>,
	"Jean-Christophe Dubois" <jcd@tribudubois.net>,
	qemu-ppc <qemu-ppc@nongnu.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>
Subject: Re: [PATCH-for-5.0 01/12] scripts/coccinelle: Add script to catch missing error_propagate() calls
Date: Thu, 26 Mar 2020 21:32:36 +0000
Message-ID: <CAFEAcA9qmwCmu-0OmNF-TkXzdd3qrhPxQRO6DS36X=Up2g4QoQ@mail.gmail.com> (raw)
In-Reply-To: <20200325191830.16553-2-f4bug@amsat.org>

On Wed, 25 Mar 2020 at 19:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> In some places in we put an error into a local Error*, but forget
> to check for failure and pass it back to the caller.
> Add a Coccinelle patch to catch automatically add the missing code.
>
> Inspired-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

This coccinelle script is impressively deep magic...
My general rule with cocci scripts is that if they serve
the purpose they're written for then that's sufficient
and they're not worth trying to polish further, but just
for my own education I have some questions below about how
this one works.

> +@match exists@
> +typedef Error;
> +Error *err;

I didn't realize you could do this kind of "this thing
must be this type" stuff in the metavariable declaration
section...

> +identifier func, errp;
> +identifier object_property_set_type1 =~ "^object_property_set_.*";
> +identifier object_property_set_type2 =~ "^object_property_set_.*";

If we relax this so that we just look for "anything that takes
an &err as its final argument" do we hit way too many false
positives ?

> +expression obj;
> +@@
> +void func(..., Error **errp)
> +{
> + <+...
> + object_property_set_type1(obj, ..., &err);
> + ... when != err

This 'when' clause means "match only when the code doesn't
touch 'err' anywhere between the two calls", right?

> + object_property_set_type2(obj, ..., &err);
> + ...+>
> +}
> +
> +@@
> +Error *match.err;
> +identifier match.errp;
> +identifier match.object_property_set_type1;
> +expression match.obj;
> +@@
> + object_property_set_type1(obj, ..., &err);
> ++if (err) {
> ++    error_propagate(errp, err);
> ++    return;
> ++}

Is there a reason we can't do the substitution
in the same rule we were using to find the match,
or was it just easier this way/done this way in
some other example you were following ?

> +
> +@manual depends on never match@
> +Error *err;
> +identifier object_property_set_type1 =~ "^object_property_set_.*";
> +identifier object_property_set_type2 =~ "^object_property_set_.*";
> +position p;
> +@@
> + object_property_set_type1@p(..., &err);
> + ... when != err
> + object_property_set_type2(..., &err);
> +
> +@script:python@
> +f << manual.object_property_set_type1;
> +p << manual.p;
> +@@
> +print("[[manual check required: "
> +      "error_propagate() might be missing in {}() {}:{}:{}]]".format(
> +            f, p[0].file, p[0].line, p[0].column))

Nice to have an example of how to do a "find these things
and print a diagnostic". This 'manual' match is handling
the cases where we got two consecutive uses of &err but
not in a function that took "Error *errp", yes?

thanks
-- PMM


  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 19:18 [PATCH-for-5.0 00/12] hw: Add missing error-propagation code Philippe Mathieu-Daudé
2020-03-25 19:18 ` [PATCH-for-5.0 01/12] scripts/coccinelle: Add script to catch missing error_propagate() calls Philippe Mathieu-Daudé
2020-03-26 21:32   ` Peter Maydell [this message]
2020-03-25 19:18 ` [PATCH-for-5.0 02/12] hw/arm/bcm2835_peripherals: Add missing error-propagation code Philippe Mathieu-Daudé
2020-03-26 21:34   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 03/12] hw/arm/fsl-imx: " Philippe Mathieu-Daudé
2020-03-26 21:35   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 04/12] hw/arm/stm32fx05_soc: " Philippe Mathieu-Daudé
2020-03-25 20:51   ` Alistair Francis
2020-03-26 21:45   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 05/12] hw/i386/x86: " Philippe Mathieu-Daudé
2020-03-26 21:38   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 06/12] hw/dma/xilinx_axidma: " Philippe Mathieu-Daudé
2020-03-25 20:52   ` Alistair Francis
2020-03-26 21:46   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 07/12] hw/mips/cps: " Philippe Mathieu-Daudé
2020-03-26 21:43   ` Peter Maydell
2020-03-26 22:48   ` Aleksandar Markovic
2020-03-25 19:18 ` [PATCH-for-5.0 08/12] hw/mips/boston: " Philippe Mathieu-Daudé
2020-03-26 21:47   ` Peter Maydell
2020-03-26 22:50   ` Aleksandar Markovic
2020-03-25 19:18 ` [PATCH-for-5.0 09/12] hw/mips/mips_malta: " Philippe Mathieu-Daudé
2020-03-26 21:49   ` Peter Maydell
2020-03-26 22:49   ` Aleksandar Markovic
2020-03-25 19:18 ` [PATCH-for-5.0 10/12] hw/misc/macio/macio: " Philippe Mathieu-Daudé
2020-03-25 23:55   ` David Gibson
2020-03-26 21:50   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 11/12] hw/net/xilinx_axienet: " Philippe Mathieu-Daudé
2020-03-25 20:52   ` Alistair Francis
2020-03-26 21:51   ` Peter Maydell
2020-03-25 19:18 ` [PATCH-for-5.0 12/12] hw/riscv/sifive_u: " Philippe Mathieu-Daudé
2020-03-25 20:52   ` Alistair Francis
2020-03-26 21:55   ` Peter Maydell
2020-03-31 17:02     ` Philippe Mathieu-Daudé
2020-03-31 17:03       ` Peter Maydell
2020-03-25 19:20 ` [PATCH-for-5.0 00/12] hw: " Philippe Mathieu-Daudé
2020-03-30  9:21   ` Stefan Hajnoczi
2020-04-06 17:47     ` Philippe Mathieu-Daudé
2020-03-31 13:23 ` Markus Armbruster
2020-04-03 17:53   ` Philippe Mathieu-Daudé
2020-04-04  5:55     ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFEAcA9qmwCmu-0OmNF-TkXzdd3qrhPxQRO6DS36X=Up2g4QoQ@mail.gmail.com' \
    --to=peter.maydell@linaro.org \
    --cc=Alistair.Francis@wdc.com \
    --cc=Andrew.Baumann@microsoft.com \
    --cc=aleksandar.qemu.devel@gmail.com \
    --cc=aleksandar.rikalo@rt-rk.com \
    --cc=alistair@alistair23.me \
    --cc=armbru@redhat.com \
    --cc=aurelien@aurel32.net \
    --cc=david@gibson.dropbear.id.au \
    --cc=edgar.iglesias@gmail.com \
    --cc=ehabkost@redhat.com \
    --cc=f4bug@amsat.org \
    --cc=jasowang@redhat.com \
    --cc=jcd@tribudubois.net \
    --cc=kbastian@mail.uni-paderborn.de \
    --cc=mark.cave-ayland@ilande.co.uk \
    --cc=mst@redhat.com \
    --cc=palmer@dabbelt.com \
    --cc=pbonzini@redhat.com \
    --cc=pburton@wavecomp.com \
    --cc=philmd@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=qemu-riscv@nongnu.org \
    --cc=rth@twiddle.net \
    --cc=sagark@eecs.berkeley.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git