Philippe Mathieu-Daudé <f4...@amsat.org> 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é <f4...@amsat.org> > --- > ...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/qemu-devel@nongnu.org/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