On 4/14/20 3:17 PM, Markus Armbruster wrote: > 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
Nothing wrong, I didn't know this form by the time I wrote this script. > > ? > > [...] >
