Re: [PATCH-for-5.1 v3 02/24] scripts/coccinelle: Script to simplify DeviceClass error propagation

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> On 4/14/20 2:24 PM, Markus Armbruster wrote:
>> 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.
>
> 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

?

[...]