Re: [Xen-devel] [PATCH v8 02/10] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>]

12.03.2020 10:23, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>
> > 11.03.2020 17:41, Markus Armbruster wrote:
> > > Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
> > >
> > > > 11.03.2020 12:38, Markus Armbruster wrote:
> > > > > Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
> > > > >
> > > > > > 09.03.2020 12:56, Markus Armbruster wrote:
> > > > > > > Suggest
> > > > > > >
> > > > > > >         scripts: Coccinelle script to use auto-propagated errp
> > > > > > >
> > > > > > > or
> > > > > > >
> > > > > > >         scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
> > > > > > >
> > > > > > > Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
> > > > > [...]
> > > > > > > > +// Note, that we update everything related to matched by rule1 function name
> > > > > > > > +// and local_err name. We may match something not related to the pattern
> > > > > > > > +// matched by rule1. For example, local_err may be defined with the same name
> > > > > > > > +// in different blocks inside one function, and in one block follow the
> > > > > > > > +// propagation pattern and in other block doesn't. Or we may have several
> > > > > > > > +// functions with the same name (for different configurations).
> > > > > > > Context: rule1 matches functions that have all three of
> > > > > > >
> > > > > > > * an Error **errp parameter
> > > > > > >
> > > > > > > * an Error *local_err = NULL variable declaration
> > > > > > >
> > > > > > > * an error_propagate(errp, local_err) or error_propagate_prepend(errp,
> > > > > > >       local_err, ...) expression, where @errp is the parameter and
> > > > > > >       @local_err is the variable.
> > > > > > >
> > > > > > > If I understand you correctly, you're pointing out two potential issues:
> > > > > > >
> > > > > > > 1. This rule can match functions rule1 does not match if there is
> > > > > > > another function with the same name that rule1 does match.
> > > > > > >
> > > > > > > 2. This rule matches in the entire function matched by rule1, even when
> > > > > > > parts of that function use a different @errp or @local_err.
> > > > > > >
> > > > > > > I figure these apply to all rules with identifier rule1.fn, not just
> > > > > > > this one.  Correct?
> > > > > > >
> > > > > > > Regarding 1.  There must be a better way to chain rules together, but I
> > > > > > > don't know it.
> > > > > > Hmm, what about something like this:
> > > > > >
> > > > > > @rule1 disable optional_qualifier exists@
> > > > > > identifier fn, local_err;
> > > > > > symbol errp;
> > > > > > @@
> > > > > >
> > > > > >     fn(..., Error **
> > > > > > - errp
> > > > > > + ___errp_coccinelle_updating___
> > > > > >        , ...)
> > > > > >     {
> > > > > >         ...
> > > > > >         Error *local_err = NULL;
> > > > > >         ...
> > > > > > (
> > > > > >        error_propagate_prepend(errp, local_err, ...);
> > > > > > |
> > > > > >        error_propagate(errp, local_err);
> > > > > > )
> > > > > >         ...
> > > > > >     }
> > > > > >
> > > > > >
> > > > > > [..]
> > > > > >
> > > > > > match symbol ___errp_coccinelle_updating___ in following rules in function 
> > > > > > header
> > > > > >
> > > > > > [..]
> > > > > >
> > > > > >
> > > > > > @ disable optional_qualifier@
> > > > > > identifier fn, local_err;
> > > > > > symbol errp;
> > > > > > @@
> > > > > >
> > > > > >     fn(..., Error **
> > > > > > - ___errp_coccinelle_updating___
> > > > > > + errp
> > > > > >        , ...)
> > > > > >     {
> > > > > >         ...
> > > > > >     }
> > > > > >
> > > > > >
> > > > > > - hacky, but seems not more hacky than python detection, and should work better
> > > > > As simple, forceful and unsubtle as a sledgehammer.  I like it :)
> > > >
> > > > Hmm, not so simple.
> > > >
> > > > It leads to reindenting of function header, which is bad.
> > > Because ___errp_coccinelle_updating___ is longer than errp, I guess.
> > > Try ____?
> > I'm afraid not. It's because it just adds \n, when I do
> >
> > ...,
> >
> > - errp
> > + ___errp_coccinelle_updating___
> > ,...
> I was thinking of something like the appended patch, which in my
> (superficial!) testing leaves alone newlines unless lines are long, but
> hangs for block.c.  Oh well.
Sorry, I didn't say, but I've checked and I was wrong: ___  works fine. But
we need --max-width 80, otherwise coccinelle wraps some lines which fit into
80 characters and produces extra hunks.

So, now I'm preparing v9 with errp->____->errp concept. It differs from your 
patch and works on block.c

We don't need to switch all errp into ____, it's an extra complication. 
Changing name only in function
header is enough. And don't worry about broken compilation: it's broken anyway 
among the rules, and all
is OK after all rules applied.

> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
> b/scripts/coccinelle/auto-propagated-errp.cocci
> index bff274bd6d..492a4db826 100644
> --- a/scripts/coccinelle/auto-propagated-errp.cocci
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -35,12 +35,12 @@
>   // error_propagate_prepend().
>   @ depends on !(file in "util/error.c") disable optional_qualifier@
>   identifier fn;
> -identifier _errp != errp;
> +identifier _errp;
>   @@
>   
>    fn(...,
>   -   Error **_errp
> -+   Error **errp
> ++   Error **____
>       ,...)
>    {
>   (
> @@ -48,7 +48,7 @@ identifier _errp != errp;
>   &
>        <...
>   -    _errp
> -+    errp
> ++    ____
>        ...>
>   )
>    }
> @@ -63,26 +63,26 @@ identifier _errp != errp;
>   // all possible control flows (otherwise, it will not match standard pattern
>   // when error_propagate() call is in if branch).
>   @ disable optional_qualifier exists@
> -identifier fn, local_err, errp;
> +identifier fn, local_err;
>   @@
>   
> - fn(..., Error **errp, ...)
> + fn(..., Error **____, ...)
>    {
>   +   ERRP_AUTO_PROPAGATE();
>       ...  when != ERRP_AUTO_PROPAGATE();
>   (
> -    error_append_hint(errp, ...);
> +    error_append_hint(____, ...);
>   |
> -    error_prepend(errp, ...);
> +    error_prepend(____, ...);
>   |
> -    error_vprepend(errp, ...);
> +    error_vprepend(____, ...);
>   |
>       Error *local_err = NULL;
>       ...
>   (
> -    error_propagate_prepend(errp, local_err, ...);
> +    error_propagate_prepend(____, local_err, ...);
>   |
> -    error_propagate(errp, local_err);
> +    error_propagate(____, local_err);
>   )
>   )
>       ... when any
> @@ -92,18 +92,17 @@ identifier fn, local_err, errp;
>   // Match scenarios with propagation of local error to errp.
>   @rule1 disable optional_qualifier exists@
>   identifier fn, local_err;
> -symbol errp;
>   @@
>   
> - fn(..., Error **errp, ...)
> + fn(..., Error **____, ...)
>    {
>        ...
>        Error *local_err = NULL;
>        ...
>   (
> -    error_propagate_prepend(errp, local_err, ...);
> +    error_propagate_prepend(____, local_err, ...);
>   |
> -    error_propagate(errp, local_err);
> +    error_propagate(____, local_err);
>   )
>        ...
>    }
> @@ -118,7 +117,6 @@ symbol errp;
>   // without error_propagate() call), coccinelle fails to match this "out: }".
>   @@
>   identifier rule1.fn, rule1.local_err, out;
> -symbol errp;
>   @@
>   
>    fn(...)
> @@ -128,7 +126,7 @@ symbol errp;
>   +    return;
>        ...>
>   - out:
> --    error_propagate(errp, local_err);
> +-    error_propagate(____, local_err);
>    }
>   
>   // Convert most of local_err related staff.
> @@ -159,7 +157,6 @@ symbol errp;
>   @ exists@
>   identifier rule1.fn, rule1.local_err;
>   expression list args;
> -symbol errp;
>   @@
>   
>    fn(...)
> @@ -172,30 +169,30 @@ symbol errp;
>   // Convert error clearing functions
>   (
>   -    error_free(local_err);
> -+    error_free_errp(errp);
> ++    error_free_errp(____);
>   |
>   -    error_report_err(local_err);
> -+    error_report_errp(errp);
> ++    error_report_errp(____);
>   |
>   -    error_reportf_err(local_err, args);
> -+    error_reportf_errp(errp, args);
> ++    error_reportf_errp(____, args);
>   |
>   -    warn_report_err(local_err);
> -+    warn_report_errp(errp);
> ++    warn_report_errp(____);
>   |
>   -    warn_reportf_err(local_err, args);
> -+    warn_reportf_errp(errp, args);
> ++    warn_reportf_errp(____, args);
>   )
>   ?-    local_err = NULL;
>   
>   |
> --    error_propagate_prepend(errp, local_err, args);
> -+    error_prepend(errp, args);
> +-    error_propagate_prepend(____, local_err, args);
> ++    error_prepend(____, args);
>   |
> --    error_propagate(errp, local_err);
> +-    error_propagate(____, local_err);
>   |
>   -    &local_err
> -+    errp
> ++    ____
>   )
>        ...>
>    }
> @@ -205,27 +202,43 @@ symbol errp;
>   // conflicts with other substitutions in it (at least with "- local_err = 
> NULL").
>   @@
>   identifier rule1.fn, rule1.local_err;
> -symbol errp;
>   @@
>   
>    fn(...)
>    {
>        <...
>   -    local_err
> -+    *errp
> ++    *____
>        ...>
>    }
>   
>   // Always use the same patter for checking error
>   @@
>   identifier rule1.fn;
> -symbol errp;
>   @@
>   
>    fn(...)
>    {
>        <...
> --    *errp != NULL
> -+    *errp
> +-    *____ != NULL
> ++    *____
>        ...>
>    }
> +
> +@@
> +identifier fn;
> +symbol errp;
> +@@
> +
> + fn(...,
> +-   Error **____
> ++   Error **errp
> +    ,...)
> + {
> + ...
> + }
> +
> +@@
> +@@
> +-____
> ++errp

--
Best regards,
Vladimir

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel