| Message ID | 20200325191830.16553-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Add missing error-propagation code | expand |
On Wed, 25 Mar 2020 at 19:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > In some places in we put an error into a local Error*, but forget > to check for failure and pass it back to the caller. > Add a Coccinelle patch to catch automatically add the missing code. > > Inspired-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- This coccinelle script is impressively deep magic... My general rule with cocci scripts is that if they serve the purpose they're written for then that's sufficient and they're not worth trying to polish further, but just for my own education I have some questions below about how this one works. > +@match exists@ > +typedef Error; > +Error *err; I didn't realize you could do this kind of "this thing must be this type" stuff in the metavariable declaration section... > +identifier func, errp; > +identifier object_property_set_type1 =~ "^object_property_set_.*"; > +identifier object_property_set_type2 =~ "^object_property_set_.*"; If we relax this so that we just look for "anything that takes an &err as its final argument" do we hit way too many false positives ? > +expression obj; > +@@ > +void func(..., Error **errp) > +{ > + <+... > + object_property_set_type1(obj, ..., &err); > + ... when != err This 'when' clause means "match only when the code doesn't touch 'err' anywhere between the two calls", right? > + object_property_set_type2(obj, ..., &err); > + ...+> > +} > + > +@@ > +Error *match.err; > +identifier match.errp; > +identifier match.object_property_set_type1; > +expression match.obj; > +@@ > + object_property_set_type1(obj, ..., &err); > ++if (err) { > ++ error_propagate(errp, err); > ++ return; > ++} Is there a reason we can't do the substitution in the same rule we were using to find the match, or was it just easier this way/done this way in some other example you were following ? > + > +@manual depends on never match@ > +Error *err; > +identifier object_property_set_type1 =~ "^object_property_set_.*"; > +identifier object_property_set_type2 =~ "^object_property_set_.*"; > +position p; > +@@ > + object_property_set_type1@p(..., &err); > + ... when != err > + object_property_set_type2(..., &err); > + > +@script:python@ > +f << manual.object_property_set_type1; > +p << manual.p; > +@@ > +print("[[manual check required: " > + "error_propagate() might be missing in {}() {}:{}:{}]]".format( > + f, p[0].file, p[0].line, p[0].column)) Nice to have an example of how to do a "find these things and print a diagnostic". This 'manual' match is handling the cases where we got two consecutive uses of &err but not in a function that took "Error *errp", yes? thanks -- PMM
diff --git a/scripts/coccinelle/object_property_missing_error_propagate.cocci b/scripts/coccinelle/object_property_missing_error_propagate.cocci new file mode 100644 index 0000000000..104e345273 --- /dev/null +++ b/scripts/coccinelle/object_property_missing_error_propagate.cocci @@ -0,0 +1,58 @@ +// Add missing error-propagation code +// +// 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/object_property_missing_error_propagate.cocci \ +// --keep-comments --smpl-spacing --in-place --dir hw +// +// Inspired by https://www.mail-archive.com/qemu-devel@nongnu.org/msg691638.html + +@match exists@ +typedef Error; +Error *err; +identifier func, errp; +identifier object_property_set_type1 =~ "^object_property_set_.*"; +identifier object_property_set_type2 =~ "^object_property_set_.*"; +expression obj; +@@ +void func(..., Error **errp) +{ + <+... + object_property_set_type1(obj, ..., &err); + ... when != err + object_property_set_type2(obj, ..., &err); + ...+> +} + +@@ +Error *match.err; +identifier match.errp; +identifier match.object_property_set_type1; +expression match.obj; +@@ + object_property_set_type1(obj, ..., &err); ++if (err) { ++ error_propagate(errp, err); ++ return; ++} + +@manual depends on never match@ +Error *err; +identifier object_property_set_type1 =~ "^object_property_set_.*"; +identifier object_property_set_type2 =~ "^object_property_set_.*"; +position p; +@@ + object_property_set_type1@p(..., &err); + ... when != err + object_property_set_type2(..., &err); + +@script:python@ +f << manual.object_property_set_type1; +p << manual.p; +@@ +print("[[manual check required: " + "error_propagate() might be missing in {}() {}:{}:{}]]".format( + f, p[0].file, p[0].line, p[0].column))
In some places in we put an error into a local Error*, but forget to check for failure and pass it back to the caller. Add a Coccinelle patch to catch automatically add the missing code. Inspired-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- ...ect_property_missing_error_propagate.cocci | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 scripts/coccinelle/object_property_missing_error_propagate.cocci