Re: [PATCH-for-5.1 v2 48/54] scripts/coccinelle: Use &error_abort in TypeInfo::instance_init()

[Vladimir Sementsov-Ogievskiy]

07.04.2020 20:27, Philippe Mathieu-Daudé wrote:
> On 4/7/20 3:07 PM, Philippe Mathieu-Daudé wrote:
> > On 4/7/20 3:01 PM, Vladimir Sementsov-Ogievskiy wrote:
> > > 07.04.2020 14:03, Philippe Mathieu-Daudé wrote:
> > > > On 4/7/20 9:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > > > 06.04.2020 20:47, Philippe Mathieu-Daudé wrote:
> > > > > > The instance_init() calls are not suppose to fail. Add a
> > > > > > Coccinelle script to use &error_abort instead of ignoring
> > > > > > errors by using a NULL Error*.
> > > > > >
> > > > > > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> > > > > > ---
> > > > > >   .../use-error_abort-in-instance_init.cocci    | 52 +++++++++++++++++++
> > > > > >   MAINTAINERS                                   |  1 +
> > > > > >   2 files changed, 53 insertions(+)
> > > > > >   create mode 100644 scripts/coccinelle/use-error_abort-in-instance_init.cocci
> > > > > >
> > > > > > diff --git a/scripts/coccinelle/use-error_abort-in-instance_init.cocci 
> > > > > > b/scripts/coccinelle/use-error_abort-in-instance_init.cocci
> > > > > > new file mode 100644
> > > > > > index 0000000000..8302d74a0c
> > > > > > --- /dev/null
> > > > > > +++ b/scripts/coccinelle/use-error_abort-in-instance_init.cocci
> > > > > > @@ -0,0 +1,52 @@
> > > > > > +// Use &error_abort in TypeInfo::instance_init()
> > > > > > +//
> > > > > > +// 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 --include-headers \
> > > > > > +//  --sp-file scripts/coccinelle/use-error_abort-in-instance_init.cocci \
> > > > > > +//  --keep-comments --in-place
> > > > > > +//
> > > > > > +// Inspired by https://www.mail-archive.com/address@hidden/msg692500.html
> > > > > > +// and https://www.mail-archive.com/address@hidden/msg693637.html
> > > > > > +
> > > > > > +
> > > > > > +@ has_qapi_error @
> > > > > > +@@
> > > > > > +    #include "qapi/error.h"
> > > > > > +
> > > > > > +
> > > > > > +@ match_instance_init @
> > > > > > +TypeInfo info;
> > > > > > +identifier instance_initfn;
> > > > > > +@@
> > > > > > +    info.instance_init = instance_initfn;
> > > > > > +
> > > > > > +
> > > > > > +@ use_error_abort @
> > > > > > +identifier match_instance_init.instance_initfn;
> > > > > > +identifier func_with_error;
> > > > > > +expression parentobj, propname, childobj, size, type, errp;
> > > > > > +position pos;
> > > > > > +@@
> > > > > > +void instance_initfn(...)
> > > > > > +{
> > > > > > +   <+...
> > > > > > +(
> > > > > > +   object_initialize_child(parentobj, propname,
> > > > > > +                           childobj, size, type,
> > > > > > +                           errp, NULL);
> > >
> > > I think, it doesn't actually differs from just object_initialize_child(..., 
> > > NULL); and you don't need all these metavaraibles
> > >
> > > > > > +|
> > > > > > +   func_with_error@pos(...,
> > > > > > +-                           NULL);
> > > > > > ++                           &error_abort);
> > > > > > +)
> > > > >
> > > > >
> > > > > Hmm. I don't follow, what are you trying to achieve by this ( | ) construction? 
> > > > > The second pattern (func_with_error...) will be matched anyway, with or without 
> > > > > first pattern (object_initialize_child...) matched. And first pattern does 
> > > > > nothing.
> > > >
> > > > Expected behavior :)
> > > >
> > > > If object_initialize_child() matched:
> > > >    do nothing.
> > > > Else:
> > > >    transform.
> > >
> > > Ah, understand, thanks. Checked, it works.
> > >
> > > Possibly simpler way is just define func_with_errno identifier like this:
> > >
> > > identifier func_with_error != object_initialize_child;
>
> Thanks for the tip Vladimir, I simplified as:
>
> @ use_error_abort @
> identifier match_instance_init.instance_initfn;
> identifier func_with_error != {qbus_create_inplace, object_initialize_child};

New syntax for me, great)

> position pos;
> @@
> void instance_initfn(...)
> {
>     <+...
>     func_with_error@pos(...,
> -                           NULL);
> +                           &error_abort);
>     ...+>
> }
>
> BTW do you know how to automatically add an include ("qapi/error.h" below)?

No, I don't.

I can guess something like this

@ already_has_include @
@@

#include <qapi/error.h>

@ depends on !already_has_include && use_error_abort @

 #include ...
+#include <qapi/error.h>

===

or maybe in one rule:

@@
... when != error.h
 #include ...
+#include <qapi/error.h>
... when != error.h


(possibly last line is not needed)..

what spec says about includes:

An #include may be followed by "...", <...> or simply .... With either quotes or 
angle brackets, it is possible to put a partial path, ending with ..., such as <include/...>, 
or to put a complete path. A #include with ... matches any include, with either quotes or angle 
brackets. Partial paths or complete are not allowed in the latter case. Something that is added 
before an include will be put before the last matching include that is not under an ifdef in the 
file. Likewise, something that is added after an include will be put after the last matching include 
that is not under an ifdef in the file.

> > > > > > +   ...+>
> > > > > > +}
> > > > > > +
> > > > > > +
> > > > > > +@script:python depends on use_error_abort && !has_qapi_error@
> > > > > > +p << use_error_abort.pos;
> > > > > > +@@
> > > > > > +print('[[manual edit required, %s misses #include "qapi/error.h"]]' % 
> > > > > > p[0].file)
> > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > > index 14de2a31dc..ae71a0a4b0 100644
> > > > > > --- a/MAINTAINERS
> > > > > > +++ b/MAINTAINERS
> > > > > > @@ -2059,6 +2059,7 @@ 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_abort-in-instance_init.cocci
> > > > > >   F: scripts/coccinelle/use-error_fatal.cocci
> > > > > >   F: scripts/coccinelle/use-error_propagate-in-realize.cocci


--
Best regards,
Vladimir