On Wed, 25 Mar 2020 at 19:18, Philippe Mathieu-Daudé <f4...@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.mayd...@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <f4...@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