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é <f4bug@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;
+    ...
+}
+
+
+@ propagate_in_realize @
+identifier match_realize.realizefn;
+identifier err;
+identifier errp;
+identifier func_with_errp =~ "(?!object_property_set_link)";
+symbol error_abort, error_fatal;
+position pos;
+@@
+void realizefn@pos(..., Error **errp)
+{
+    ...
+    Error *err = NULL;
+    <+...
+    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
-- 
2.21.1

Philippe Mathieu-Daudé <f4bug@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é <f4bug@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

On 4/14/20 2:24 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@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é <f4bug@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()"?

> 
>> +symbol error_abort, error_fatal;
>> +position pos;
>> +@@
>> +void realizefn@pos(..., Error **errp)
>> +{
>> +    ...
>> +    Error *err = NULL;
> 
> Why is this line required for a match?

Hmmm maybe I was expecting a local_err... else a left-over from previous 
intents :)

> 
>> +    <+...
>> +    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
> 

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/14/20 2:24 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@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é <f4bug@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

?

[...]

On 4/14/20 3:17 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 4/14/20 2:24 PM, Markus Armbruster wrote:
>>> Philippe Mathieu-Daudé <f4bug@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é <f4bug@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.

> 
> ?
> 
> [...]
>