On Wed, 25 Mar 2020 at 19:18, Philippe Mathieu-Daudé 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
> Signed-off-by: Philippe Mathieu-Daudé
> ---
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 as its final argument" do we hit way too many false
positives ?
> +expression obj;
> +@@
> +void func(..., Error **errp)
> +{
> + <+...
> + object_property_set_type1(obj, ..., );
> + ... 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, ..., );
> + ...+>
> +}
> +
> +@@
> +Error *match.err;
> +identifier match.errp;
> +identifier match.object_property_set_type1;
> +expression match.obj;
> +@@
> + object_property_set_type1(obj, ..., );
> ++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(..., );
> + ... when != err
> + object_property_set_type2(..., );
> +
> +@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 but
not in a function that took "Error *errp", yes?
thanks
-- PMM
