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

[Philippe Mathieu-Daudé]

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()"?

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