On Tue, Apr 7, 2020 at 7:55 PM Vladimir Sementsov-Ogievskiy
<vsement...@virtuozzo.com> 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é <f4...@amsat.org>
> >>>>>> ---
> >>>>>> .../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/qemu-devel@nongnu.org/msg692500.html
> >>>>>> +// and
> >>>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/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
>
