Philippe Mathieu-Daudé <phi...@redhat.com> writes: > On 4/14/20 2:24 PM, Markus Armbruster wrote: >> 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. > > OK. > >> >>> + >>> + >>> +@ 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? > > "match all func_with_errp() except object_property_set_link()"?
What's wrong with identifier func_with_errp != object_property_set_link ? [...]