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

[Philippe Mathieu-Daudé]

On 4/14/20 3:17 PM, Markus Armbruster wrote:
> 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

Nothing wrong, I didn't know this form by the time I wrote this script.

> 
> ?
> 
> [...]
>