[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH-for-5.1 v3 02/24] scripts/coccinelle: Script to simplify Devi
From: |
Markus Armbruster |
Subject: |
Re: [PATCH-for-5.1 v3 02/24] scripts/coccinelle: Script to simplify DeviceClass error propagation |
Date: |
Tue, 14 Apr 2020 14:24:07 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Philippe Mathieu-Daudé <address@hidden> writes:
> When a device uses an Error* with data not modified before realize(),
> this call can be moved to init(). Add a Coccinelle patch to find such
> uses.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> ...implify-init-realize-error_propagate.cocci | 69 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 70 insertions(+)
> create mode 100644
> scripts/coccinelle/simplify-init-realize-error_propagate.cocci
>
> diff --git a/scripts/coccinelle/simplify-init-realize-error_propagate.cocci
> b/scripts/coccinelle/simplify-init-realize-error_propagate.cocci
> new file mode 100644
> index 0000000000..2e3ec4d98a
> --- /dev/null
> +++ b/scripts/coccinelle/simplify-init-realize-error_propagate.cocci
> @@ -0,0 +1,69 @@
> +// Find error-propagation calls that don't need to be in
> DeviceClass::realize()
> +// because they don't use information user can change before calling
> realize(),
> +// so they can be moved to DeviceClass:initfn() where error propagation is
> not
> +// needed.
> +//
> +// Copyright: (C) 2020 Philippe Mathieu-Daudé
> +// This work is licensed under the terms of the GNU GPLv2 or later.
> +//
> +// spatch \
> +// --macro-file scripts/cocci-macro-file.h \
> +// --sp-file \
> +// scripts/coccinelle/simplify-init-realize-error_propagate.cocci \
> +// --timeout 60
> +//
> +// Inspired by https://www.mail-archive.com/address@hidden/msg692500.html
> +
> +
> +@ match_class_init @
> +TypeInfo info;
> +identifier class_initfn;
> +@@
> + info.class_init = class_initfn;
> +
> +
> +@ match_instance_init @
> +TypeInfo info;
> +identifier instance_initfn;
> +@@
> + info.instance_init = instance_initfn;
> +
> +
> +@ match_realize @
> +identifier match_class_init.class_initfn;
> +DeviceClass *dc;
> +identifier realizefn;
> +@@
> +void class_initfn(...)
> +{
> + ...
> + dc->realize = realizefn;
> + ...
> +}
I'm afraid this misses realize() methods of DeviceClass subclasses.
Consider PCI device "i6300esb" (picked just because it's simple).
pci_device_class_init() sets DeviceClass method realize() to
pci_qdev_realize(). pci_qdev_realize() does the work common to all PCI
devices, and calls PCIDeviceClass method realize() for the work specific
to the PCI device at hand.
i6300esb_class_init() sets PCIDeviceClass method realize() to
i6300esb_realize().
Your first rule should match i6300esb_info alright, and thus identify
i6300esb_class_init() as a class_init() method.
But your third rule can't match i6300esb_class_init()'s
k->realize = i6300esb_realize;
because @k is a PCIDeviceClass, not a DeviceClass.
I think it also misses cases that have a realize(), but no
instance_init().
Finding only some instances of an anti-pattern can still be useful. But
you should explain the script's limitations then, both in the script and
the commit message.
> +
> +
> +@ propagate_in_realize @
> +identifier match_realize.realizefn;
> +identifier err;
> +identifier errp;
> +identifier func_with_errp =~ "(?!object_property_set_link)";
What are you trying to accomplish with this lookahead assertion?
> +symbol error_abort, error_fatal;
> +position pos;
> +@@
> +void realizefn@pos(..., Error **errp)
> +{
> + ...
> + Error *err = NULL;
Why is this line required for a match?
> + <+...
> + func_with_errp(..., \(&error_abort\|&error_fatal\));
> + ...+>
> +}
> +
> +
> +@ script:python @
> +initfn << match_instance_init.instance_initfn;
> +realizefn << match_realize.realizefn;
> +p << propagate_in_realize.pos;
> +@@
> +print('>>> possible moves from {}() to {}() in {}:{}'
> + .format(initfn, realizefn, p[0].file, p[0].line))
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 642c8e0b6b..6203543ec0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2058,6 +2058,7 @@ F: scripts/coccinelle/err-bad-newline.cocci
> F: scripts/coccinelle/error-use-after-free.cocci
> F: scripts/coccinelle/error_propagate_null.cocci
> F: scripts/coccinelle/remove_local_err.cocci
> +F: scripts/coccinelle/simplify-init-realize-error_propagate.cocci
> F: scripts/coccinelle/use-error_fatal.cocci
>
> GDB stub
[PATCH-for-5.1 v3 03/24] hw/arm/allwinner-a10: Move some code from realize() to init(), Philippe Mathieu-Daudé, 2020/04/12
[PATCH-for-5.1 v3 01/24] various: Remove suspicious '\' character outside of #define in C code, Philippe Mathieu-Daudé, 2020/04/12
[PATCH-for-5.1 v3 04/24] hw/arm/aspeed_ast2600: Simplify use of Error*, Philippe Mathieu-Daudé, 2020/04/12