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

[Philippe Mathieu-Daudé]

On Tue, Apr 7, 2020 at 7:55 PM Vladimir Sementsov-Ogievskiy
<address@hidden> wrote:
>
> 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>

OMG this works!

@ depends on !has_qapi_error && use_error_abort @
@@
    #include ...
+   #include "qapi/error.h"

Produces:

diff -u -p a/e1000.c b/e1000.c
--- a/e1000.c
+++ b/e1000.c
@@ -39,6 +39,7 @@

 #include "e1000x_common.h"
 #include "trace.h"
+#include "qapi/error.h"

 static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};

@@ -1774,7 +1775,7 @@ static void e1000_instance_init(Object *
     E1000State *n = E1000(obj);
     device_add_bootindex_property(obj, &n->conf.bootindex,
                                   "bootindex", "/ethernet-phy@0",
-                                  DEVICE(n), NULL);
+                                  DEVICE(n), &error_abort);
 }


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