[v7,03/11] scripts: add coccinelle script to use auto propagated errp

Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)

Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
 --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
 blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

CC: Eric Blake <eblake@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Greg Kurz <groug@kaod.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Anthony Perard <anthony.perard@citrix.com>
CC: Paul Durrant <paul@xen.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
CC: Laszlo Ersek <lersek@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Stefan Berger <stefanb@linux.ibm.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: qemu-block@nongnu.org
CC: xen-devel@lists.xenproject.org

 include/qapi/error.h                          |   3 +
 scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
 2 files changed, 161 insertions(+)
 create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
> does corresponding changes in code (look for details in
> include/qapi/error.h)
>
> Usage example:
> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> CC: Eric Blake <eblake@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Greg Kurz <groug@kaod.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Anthony Perard <anthony.perard@citrix.com>
> CC: Paul Durrant <paul@xen.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> CC: Stefan Berger <stefanb@linux.ibm.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> CC: qemu-block@nongnu.org
> CC: xen-devel@lists.xenproject.org
>
>  include/qapi/error.h                          |   3 +
>  scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>  2 files changed, 161 insertions(+)
>  create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b9452d4806..79f8e95214 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -141,6 +141,9 @@
>   *         ...
>   *     }
>   *
> + * For mass conversion use script
> + *   scripts/coccinelle/auto-propagated-errp.cocci
> + *
>   *
>   * Receive and accumulate multiple errors (first one wins):
>   *     Error *err = NULL, *local_err = NULL;

Extra blank line.

> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
> new file mode 100644
> index 0000000000..fb03c871cb
> --- /dev/null
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -0,0 +1,158 @@
> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
> +//
> +// Copyright (c) 2020 Virtuozzo International GmbH.
> +//
> +// This program is free software; you can redistribute it and/or modify
> +// it under the terms of the GNU General Public License as published by
> +// the Free Software Foundation; either version 2 of the License, or
> +// (at your option) any later version.
> +//
> +// This program is distributed in the hope that it will be useful,
> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +// GNU General Public License for more details.
> +//
> +// You should have received a copy of the GNU General Public License
> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +//
> +// Usage example:
> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
> +
> +@rule0@
> +// Add invocation to errp-functions where necessary
> +// We should skip functions with "Error *const *errp"
> +// parameter, but how to do it with coccinelle?
> +// I don't know, so, I skip them by function name regex.
> +// It's safe: if we did not skip some functions with
> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
> +// will fail to compile, because of const violation.

Not skipping a function we should skip fails to compile.

What about skipping a function we should not skip?

> +identifier fn !~ "error_append_.*_hint";
> +identifier local_err, ERRP;

A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
don't.  Either is fine with me.  Mixing the two styles feels a bit
confusing, though.

> +@@
> +
> + fn(..., Error **ERRP, ...)
> + {
> ++   ERRP_AUTO_PROPAGATE();
> +    <+...
> +        when != ERRP_AUTO_PROPAGATE();
> +(
> +    error_append_hint(ERRP, ...);
> +|
> +    error_prepend(ERRP, ...);
> +|
> +    Error *local_err = NULL;
> +)
> +    ...+>
> + }

Misses error_vprepend().  Currently harmless, but as long as we commit
the script, we better make it as robust as we reasonably can.

The previous patch explains this Coccinelle script's intent:

  To achieve these goals, later patches will add invocations
  of this macro at the start of functions with either use
  error_prepend/error_append_hint (solving 1) or which use
  local_err+error_propagate to check errors, switching those
  functions to use *errp instead (solving 2 and 3).

This rule matches "use error_prepend/error_append_hint" directly.  It
appears to use presence of a local Error * variable as proxy for "use
local_err+error_propagate to check errors".  Hmm.

We obviously have such a variable when we use "local_err+error_propagate
to check errors".  But we could also have such variables without use of
error_propagate().  In fact, error.h documents such use:

 * Call a function and receive an error from it:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *     }

where "handle the error" frees it.

I figure such uses typically occur in functions without an Error **errp
parameter.  This rule doesn't apply then.  But they could occur even in
functions with such a parameter.  Consider:

    void foo(Error **errp)
    {
        Error *err = NULL;

        bar(&err);
        if (err) {
            error_free(err);
            error_setg(errp, "completely different error");
        }
    }

Reasonable enough when bar() gives us an error that's misleading in this
context, isn't it?

The script transforms it like this:

    void foo(Error **errp)
    {
   -    Error *err = NULL;
   +    ERRP_AUTO_PROPAGATE();

   -    bar(&err);
   -    if (err) {
   -        error_free(err);
   +    bar(errp);
   +    if (*errp) {
   +        error_free_errp(errp);
            error_setg(errp, "completely different error");
        }
    }

Unwanted.

Now, if this script applied in just a few dozen places, we could rely on
eyeballing its output to catch unwanted transformations.  Since it
applies in so many more, I don't feel comfortable relying on reviewer
eyeballs.

Can we make rule0 directly match error_propagate(errp, local_err)
somehow?

Another observation: the rule does not match error_reportf_err() and
warn_reportf_err().  These combine error_prepend(),
error_report()/warn_report() and error_free(), for convenience.  Don't
their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
users?

> +
> +@@
> +// Switch unusual (Error **) parameter names to errp
> +// (this is necessary to use ERRP_AUTO_PROPAGATE).

Please put your rule comments right before the rule, i.e. before the
@-line introducing metavariable declarations, not after.  Same
elsewhere.

> +identifier rule0.fn;
> +identifier rule0.ERRP != errp;
> +@@
> +
> + fn(...,
> +-   Error **ERRP
> ++   Error **errp
> +    ,...)
> + {
> +     <...
> +-    ERRP
> ++    errp
> +     ...>
> + }

This normalizes errp parameter naming.  It matches exactly when rule0
matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
is unusual.  Good.

> +
> +@rule1@
> +// We want to patch error propagation in functions regardless of
> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
> +// applying rule0, hence this one does not inherit from it.

I'm not sure I get this comment.  Let's see what the rule does.

> +identifier fn !~ "error_append_.*_hint";
> +identifier local_err;
> +symbol errp;
> +@@
> +
> + fn(..., Error **errp, ...)
> + {
> +     <...
> +-    Error *local_err = NULL;
> +     ...>
> + }

rule1 matches like rule0, except the Error ** parameter match is
tightened from any C identifier to the C identifier errp, and the
function body match tightened from "either use
error_prepend/error_append_hint or which use local_err+error_propagate
to check errors" to just the latter.

I figure tightening the Error ** parameter match has no effect, because
we already normalized the parameter name.

So rule1 deletes variable local_err where rule0 applied.  Correct?

> +
> +@@
> +// Handle pattern with goto, otherwise we'll finish up
> +// with labels at function end which will not compile.
> +identifier rule1.fn, rule1.local_err;
> +identifier OUT;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +-    goto OUT;
> ++    return;
> +     ...>
> +- OUT:
> +-    error_propagate(errp, local_err);
> + }

This is one special case of error_propagate() deletion.  It additionally
gets rid of a goto we no longer want.  For the general case, see below.

The rule applies only where rule1 just deleted the variable.  Thus, the
two rules work in tandem.  Makes sense.

> +
> +@@
> +identifier rule1.fn, rule1.local_err;

This rule also works in tandem with rule1.

> +expression list args; // to reindent error_propagate_prepend

What is the comment trying to tell me?

> +@@
> +
> + fn(...)
> + {
> +     <...
> +(
> +-    error_free(local_err);
> +-    local_err = NULL;
> ++    error_free_errp(errp);

Reminder:

    static inline void error_free_errp(Error **errp)
    {
        assert(errp && *errp);
        error_free(*errp);
        *errp = NULL;
    }

Now let's examine the actual change.

The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
ensures it.

The second half is new.  We now crash when we haven't set an error.  Why
is this safe?  Note that error_free(local_err) does nothing when
!local_err.

The zapping of the variable pointing to the Error just freed is
unchanged.

> +|
> +-    error_free(local_err);
> ++    error_free_errp(errp);

Here, the zapping is new.  Zapping dangling pointers is obviously safe.
Needed, or else the automatic error_propagate() due to
ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.

> +|
> +-    error_report_err(local_err);
> ++    error_report_errp(errp);

The only difference to the previous case is that we also report the
error.

The previous case has a buddy that additionally matches *errp = NULL.
Why not this one?

> +|
> +-    warn_report_err(local_err);
> ++    warn_report_errp(errp);

Likewise.

What about error_reportf_err(), warn_reportf_err()?

Up to here, this rule transforms the various forms of error_free().
Next: error_propagate().

> +|
> +-    error_propagate_prepend(errp, local_err, args);
> ++    error_prepend(errp, args);
> +|
> +-    error_propagate(errp, local_err);

rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
redundant.

This is the general case of error_propagate() deletion.

I'd put the plain error_propagate() first, variations second, like you
do with error_free().

If neither of these two patterns match on a path from
ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
where it wasn't before.  Does nothing when the local error is null
there.  Bug fix when it isn't: it's at least a memory leak, and quite
possibly worse.

Identifying these bug fixes would be nice, but I don't have practical
ideas on how to do that.

Can we explain this in the commit message?

> +)
> +     ...>
> + }
> +
> +@@
> +identifier rule1.fn, rule1.local_err;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +(
> +-    &local_err
> ++    errp
> +|
> +-    local_err
> ++    *errp
> +)
> +     ...>
> + }

Also in tandem with rule1, fixes up uses of local_err.  Good.

> +
> +@@
> +identifier rule1.fn;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +- *errp != NULL
> ++ *errp
> +     ...>
> + }

Still in tandem with rule1, normalizes style.  Good.

23.02.2020 11:55, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>> does corresponding changes in code (look for details in
>> include/qapi/error.h)
>>
>> Usage example:
>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> CC: Greg Kurz <groug@kaod.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Anthony Perard <anthony.perard@citrix.com>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> CC: Stefan Berger <stefanb@linux.ibm.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> CC: qemu-block@nongnu.org
>> CC: xen-devel@lists.xenproject.org
>>
>>   include/qapi/error.h                          |   3 +
>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>   2 files changed, 161 insertions(+)
>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index b9452d4806..79f8e95214 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -141,6 +141,9 @@
>>    *         ...
>>    *     }
>>    *
>> + * For mass conversion use script
>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>> + *
>>    *
>>    * Receive and accumulate multiple errors (first one wins):
>>    *     Error *err = NULL, *local_err = NULL;
> 
> Extra blank line.
> 
>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>> new file mode 100644
>> index 0000000000..fb03c871cb
>> --- /dev/null
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>> @@ -0,0 +1,158 @@
>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>> +//
>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>> +//
>> +// This program is free software; you can redistribute it and/or modify
>> +// it under the terms of the GNU General Public License as published by
>> +// the Free Software Foundation; either version 2 of the License, or
>> +// (at your option) any later version.
>> +//
>> +// This program is distributed in the hope that it will be useful,
>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +// GNU General Public License for more details.
>> +//
>> +// You should have received a copy of the GNU General Public License
>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +//
>> +// Usage example:
>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>> +
>> +@rule0@
>> +// Add invocation to errp-functions where necessary
>> +// We should skip functions with "Error *const *errp"
>> +// parameter, but how to do it with coccinelle?
>> +// I don't know, so, I skip them by function name regex.
>> +// It's safe: if we did not skip some functions with
>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>> +// will fail to compile, because of const violation.
> 
> Not skipping a function we should skip fails to compile.
> 
> What about skipping a function we should not skip?

Then it will not be updated.. Not good but I don't have better solution.
Still, I hope, function called *error_append_*_hint will not return error
through errp pointer.

> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err, ERRP;
> 
> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
> don't.  Either is fine with me.  Mixing the two styles feels a bit
> confusing, though.
> 
>> +@@
>> +
>> + fn(..., Error **ERRP, ...)
>> + {
>> ++   ERRP_AUTO_PROPAGATE();
>> +    <+...
>> +        when != ERRP_AUTO_PROPAGATE();
>> +(
>> +    error_append_hint(ERRP, ...);
>> +|
>> +    error_prepend(ERRP, ...);
>> +|
>> +    Error *local_err = NULL;
>> +)
>> +    ...+>
>> + }
> 
> Misses error_vprepend().  Currently harmless, but as long as we commit
> the script, we better make it as robust as we reasonably can.
> 
> The previous patch explains this Coccinelle script's intent:
> 
>    To achieve these goals, later patches will add invocations
>    of this macro at the start of functions with either use
>    error_prepend/error_append_hint (solving 1) or which use
>    local_err+error_propagate to check errors, switching those
>    functions to use *errp instead (solving 2 and 3).
> 
> This rule matches "use error_prepend/error_append_hint" directly.  It
> appears to use presence of a local Error * variable as proxy for "use
> local_err+error_propagate to check errors".  Hmm.
> 
> We obviously have such a variable when we use "local_err+error_propagate
> to check errors".  But we could also have such variables without use of
> error_propagate().  In fact, error.h documents such use:
> 
>   * Call a function and receive an error from it:
>   *     Error *err = NULL;
>   *     foo(arg, &err);
>   *     if (err) {
>   *         handle the error...
>   *     }
> 
> where "handle the error" frees it.
> 
> I figure such uses typically occur in functions without an Error **errp
> parameter.  This rule doesn't apply then.  But they could occur even in
> functions with such a parameter.  Consider:
> 
>      void foo(Error **errp)
>      {
>          Error *err = NULL;
> 
>          bar(&err);
>          if (err) {
>              error_free(err);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Reasonable enough when bar() gives us an error that's misleading in this
> context, isn't it?
> 
> The script transforms it like this:
> 
>      void foo(Error **errp)
>      {
>     -    Error *err = NULL;
>     +    ERRP_AUTO_PROPAGATE();
> 
>     -    bar(&err);
>     -    if (err) {
>     -        error_free(err);
>     +    bar(errp);
>     +    if (*errp) {
>     +        error_free_errp(errp);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Unwanted.

What is the problem with it? Updated code just use "new usual notation"
for handling error of sub-calls in function which reports errors through
errp pointer.

> 
> Now, if this script applied in just a few dozen places, we could rely on
> eyeballing its output to catch unwanted transformations.  Since it
> applies in so many more, I don't feel comfortable relying on reviewer
> eyeballs.
> 
> Can we make rule0 directly match error_propagate(errp, local_err)
> somehow?

I think it is possible, still I'm not sure we need it.

> 
> Another observation: the rule does not match error_reportf_err() and
> warn_reportf_err().  These combine error_prepend(),
> error_report()/warn_report() and error_free(), for convenience.  Don't
> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
> users?

Right. These functions want to add information, which will not work
for error_fatal without wrapping.

> 
>> +
>> +@@
>> +// Switch unusual (Error **) parameter names to errp
>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
> 
> Please put your rule comments right before the rule, i.e. before the
> @-line introducing metavariable declarations, not after.  Same
> elsewhere.
> 
>> +identifier rule0.fn;
>> +identifier rule0.ERRP != errp;
>> +@@
>> +
>> + fn(...,
>> +-   Error **ERRP
>> ++   Error **errp
>> +    ,...)
>> + {
>> +     <...
>> +-    ERRP
>> ++    errp
>> +     ...>
>> + }
> 
> This normalizes errp parameter naming.  It matches exactly when rule0
> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
> is unusual.  Good.
> 
>> +
>> +@rule1@
>> +// We want to patch error propagation in functions regardless of
>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>> +// applying rule0, hence this one does not inherit from it.
> 
> I'm not sure I get this comment.  Let's see what the rule does.
> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err;
>> +symbol errp;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> +     <...
>> +-    Error *local_err = NULL;
>> +     ...>
>> + }
> 
> rule1 matches like rule0, except the Error ** parameter match is
> tightened from any C identifier to the C identifier errp, and the
> function body match tightened from "either use
> error_prepend/error_append_hint or which use local_err+error_propagate
> to check errors" to just the latter.
> 
> I figure tightening the Error ** parameter match has no effect, because
> we already normalized the parameter name.
> 
> So rule1 deletes variable local_err where rule0 applied.  Correct?

The difference with rule0 is that rule0 contains
  "when != ERRP_AUTO_PROPAGATE()", so rule0 is not applied where
we already have macro invocation.

This is why we can't inherit from rule0.

No we believe that we have ERRP_AUTO_PROPAGATE invocation in all
corresponding places (added by rule0 or before script run) and want to
update all usage of local_err objects.

> 
>> +
>> +@@
>> +// Handle pattern with goto, otherwise we'll finish up
>> +// with labels at function end which will not compile.
>> +identifier rule1.fn, rule1.local_err;
>> +identifier OUT;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +-    goto OUT;
>> ++    return;
>> +     ...>
>> +- OUT:
>> +-    error_propagate(errp, local_err);
>> + }
> 
> This is one special case of error_propagate() deletion.  It additionally
> gets rid of a goto we no longer want.  For the general case, see below.
> 
> The rule applies only where rule1 just deleted the variable.  Thus, the
> two rules work in tandem.  Makes sense.
> 
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
> 
> This rule also works in tandem with rule1.
> 
>> +expression list args; // to reindent error_propagate_prepend
> 
> What is the comment trying to tell me?

Hmm, we can safely drop it. It's about the following:

instead of

  -    error_propagate_prepend(errp, local_err, args);
  +    error_prepend(errp, args);

we can use "...", like

  - error_propagate_prepend(errp, local_err
  + error_prepend(errp
    , ...);

but with metavar in use, coccinelle will correctly reindent the
whole call, which looks a lot better.

> 
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    error_free(local_err);
>> +-    local_err = NULL;
>> ++    error_free_errp(errp);
> 
> Reminder:
> 
>      static inline void error_free_errp(Error **errp)
>      {
>          assert(errp && *errp);
>          error_free(*errp);
>          *errp = NULL;
>      }
> 
> Now let's examine the actual change.
> 
> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
> ensures it.
> 
> The second half is new.  We now crash when we haven't set an error.  Why
> is this safe?  Note that error_free(local_err) does nothing when
> !local_err.

Hmm. Looks like we should tighten this restriction, and follow error_free
interface, which allows freeing unset errp.

> 
> The zapping of the variable pointing to the Error just freed is
> unchanged.
> 
>> +|
>> +-    error_free(local_err);
>> ++    error_free_errp(errp);
> 
> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
> Needed, or else the automatic error_propagate() due to
> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
> 
>> +|
>> +-    error_report_err(local_err);
>> ++    error_report_errp(errp);
> 
> The only difference to the previous case is that we also report the
> error.
> 
> The previous case has a buddy that additionally matches *errp = NULL.
> Why not this one?

Probably because no matches in code. But should be added here for
better case coverage.

> 
>> +|
>> +-    warn_report_err(local_err);
>> ++    warn_report_errp(errp);
> 
> Likewise.
> 
> What about error_reportf_err(), warn_reportf_err()?
> 
> Up to here, this rule transforms the various forms of error_free().
> Next: error_propagate().
> 
>> +|
>> +-    error_propagate_prepend(errp, local_err, args);
>> ++    error_prepend(errp, args);
>> +|
>> +-    error_propagate(errp, local_err);
> 
> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
> redundant.
> 
> This is the general case of error_propagate() deletion.
> 
> I'd put the plain error_propagate() first, variations second, like you
> do with error_free().
> 
> If neither of these two patterns match on a path from
> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
> where it wasn't before.  Does nothing when the local error is null
> there.  Bug fix when it isn't: it's at least a memory leak, and quite
> possibly worse.

Hmm. How can it be memory leak after any of error_free variants?

> 
> Identifying these bug fixes would be nice, but I don't have practical
> ideas on how to do that.
> 
> Can we explain this in the commit message?
> 
>> +)
>> +     ...>
>> + }
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    &local_err
>> ++    errp
>> +|
>> +-    local_err
>> ++    *errp
>> +)
>> +     ...>
>> + }
> 
> Also in tandem with rule1, fixes up uses of local_err.  Good.
> 
>> +
>> +@@
>> +identifier rule1.fn;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +- *errp != NULL
>> ++ *errp
>> +     ...>
>> + }
> 
> Still in tandem with rule1, normalizes style.  Good.
>

23.02.2020 11:55, Markus Armbruster wrote:
>> +|
>> +-    warn_report_err(local_err);
>> ++    warn_report_errp(errp);
> Likewise.
> 
> What about error_reportf_err(), warn_reportf_err()?

Hmm I'm afraid, we don't have corresponding cases to update..

We can still handle them here, but, then, should we use nonexistent error_reportf_errp, so if it matched, we'll have compilation error?
Or may be coccinelle has some kind of "abort()" on the match, to error-out that "please define error_reportf_errp and update coccinelle script first"...

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 23.02.2020 11:55, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>> does corresponding changes in code (look for details in
>>> include/qapi/error.h)
>>>
>>> Usage example:
>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>
>>> CC: Eric Blake <eblake@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Max Reitz <mreitz@redhat.com>
>>> CC: Greg Kurz <groug@kaod.org>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Anthony Perard <anthony.perard@citrix.com>
>>> CC: Paul Durrant <paul@xen.org>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>> CC: Laszlo Ersek <lersek@redhat.com>
>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>> CC: Stefan Berger <stefanb@linux.ibm.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> CC: qemu-block@nongnu.org
>>> CC: xen-devel@lists.xenproject.org
>>>
>>>   include/qapi/error.h                          |   3 +
>>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>   2 files changed, 161 insertions(+)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index b9452d4806..79f8e95214 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -141,6 +141,9 @@
>>>    *         ...
>>>    *     }
>>>    *
>>> + * For mass conversion use script
>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>> + *
>>>    *
>>>    * Receive and accumulate multiple errors (first one wins):
>>>    *     Error *err = NULL, *local_err = NULL;
>>
>> Extra blank line.
>>
>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>> new file mode 100644
>>> index 0000000000..fb03c871cb
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>> @@ -0,0 +1,158 @@
>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>> +//
>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>> +//
>>> +// This program is free software; you can redistribute it and/or modify
>>> +// it under the terms of the GNU General Public License as published by
>>> +// the Free Software Foundation; either version 2 of the License, or
>>> +// (at your option) any later version.
>>> +//
>>> +// This program is distributed in the hope that it will be useful,
>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +// GNU General Public License for more details.
>>> +//
>>> +// You should have received a copy of the GNU General Public License
>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +//
>>> +// Usage example:
>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>> +
>>> +@rule0@
>>> +// Add invocation to errp-functions where necessary
>>> +// We should skip functions with "Error *const *errp"
>>> +// parameter, but how to do it with coccinelle?
>>> +// I don't know, so, I skip them by function name regex.
>>> +// It's safe: if we did not skip some functions with
>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>> +// will fail to compile, because of const violation.
>>
>> Not skipping a function we should skip fails to compile.
>>
>> What about skipping a function we should not skip?
>
> Then it will not be updated.. Not good but I don't have better solution.
> Still, I hope, function called *error_append_*_hint will not return error
> through errp pointer.

Seems likely.  I just dislike inferring behavior from name patterns.

Ideally, we recognize the true exceptional pattern instead, i.e. the
presence of const.  But figuring out how to make Coccinelle do that for
us may be more trouble than it's worth.

Hmm...  Coccinelle matches the parameter even with const due to what it
calls "isomorphism".  Can I disable it?  *Tinker* *tinker*

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
index fb03c871cb..0c4414bff3 100644
--- a/scripts/coccinelle/auto-propagated-errp.cocci
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -20,15 +20,11 @@
 //  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
 //  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
 
-@rule0@
+@rule0 disable optional_qualifier@
 // Add invocation to errp-functions where necessary
-// We should skip functions with "Error *const *errp"
-// parameter, but how to do it with coccinelle?
-// I don't know, so, I skip them by function name regex.
-// It's safe: if we did not skip some functions with
-// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
-// will fail to compile, because of const violation.
-identifier fn !~ "error_append_.*_hint";
+// Disable optional_qualifier to skip functions with "Error *const *errp"
+// parameter, 
+identifier fn;
 identifier local_err, ERRP;
 @@

Could you verify this results in the same tree-wide change as your
version?

>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err, ERRP;
>>
>> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
>> don't.  Either is fine with me.  Mixing the two styles feels a bit
>> confusing, though.
>>
>>> +@@
>>> +
>>> + fn(..., Error **ERRP, ...)
>>> + {
>>> ++   ERRP_AUTO_PROPAGATE();
>>> +    <+...
>>> +        when != ERRP_AUTO_PROPAGATE();
>>> +(
>>> +    error_append_hint(ERRP, ...);
>>> +|
>>> +    error_prepend(ERRP, ...);
>>> +|
>>> +    Error *local_err = NULL;
>>> +)
>>> +    ...+>
>>> + }
>>
>> Misses error_vprepend().  Currently harmless, but as long as we commit
>> the script, we better make it as robust as we reasonably can.
>>
>> The previous patch explains this Coccinelle script's intent:
>>
>>    To achieve these goals, later patches will add invocations
>>    of this macro at the start of functions with either use
>>    error_prepend/error_append_hint (solving 1) or which use
>>    local_err+error_propagate to check errors, switching those
>>    functions to use *errp instead (solving 2 and 3).
>>
>> This rule matches "use error_prepend/error_append_hint" directly.  It
>> appears to use presence of a local Error * variable as proxy for "use
>> local_err+error_propagate to check errors".  Hmm.
>>
>> We obviously have such a variable when we use "local_err+error_propagate
>> to check errors".  But we could also have such variables without use of
>> error_propagate().  In fact, error.h documents such use:
>>
>>   * Call a function and receive an error from it:
>>   *     Error *err = NULL;
>>   *     foo(arg, &err);
>>   *     if (err) {
>>   *         handle the error...
>>   *     }
>>
>> where "handle the error" frees it.
>>
>> I figure such uses typically occur in functions without an Error **errp
>> parameter.  This rule doesn't apply then.  But they could occur even in
>> functions with such a parameter.  Consider:
>>
>>      void foo(Error **errp)
>>      {
>>          Error *err = NULL;
>>
>>          bar(&err);
>>          if (err) {
>>              error_free(err);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Reasonable enough when bar() gives us an error that's misleading in this
>> context, isn't it?
>>
>> The script transforms it like this:
>>
>>      void foo(Error **errp)
>>      {
>>     -    Error *err = NULL;
>>     +    ERRP_AUTO_PROPAGATE();
>>
>>     -    bar(&err);
>>     -    if (err) {
>>     -        error_free(err);
>>     +    bar(errp);
>>     +    if (*errp) {
>>     +        error_free_errp(errp);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Unwanted.
>
> What is the problem with it? Updated code just use "new usual notation"
> for handling error of sub-calls in function which reports errors through
> errp pointer.

error.h's big comment asks for use of ERRP_AUTO_PROPAGATE() to "Receive
an error and pass it on to the caller".  We're not doing that here.  We
"Call a function and receive an error from it", then "Handle an error
without reporting it".

The updated code works anyway, but it's needlessly complicated.

>> Now, if this script applied in just a few dozen places, we could rely on
>> eyeballing its output to catch unwanted transformations.  Since it
>> applies in so many more, I don't feel comfortable relying on reviewer
>> eyeballs.
>>
>> Can we make rule0 directly match error_propagate(errp, local_err)
>> somehow?
>
> I think it is possible, still I'm not sure we need it.

We don't need it in the sense of "must have to avoid a buggy
transformation".  It's more like "I'd like to have it to stay close to
the documented usage of ERRP_AUTO_PROPAGATE(), and to avoid complicating
cases like the one above".

>> Another observation: the rule does not match error_reportf_err() and
>> warn_reportf_err().  These combine error_prepend(),
>> error_report()/warn_report() and error_free(), for convenience.  Don't
>> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
>> users?
>
> Right. These functions want to add information, which will not work
> for error_fatal without wrapping.

A simple improvement, I hope.

>>> +
>>> +@@
>>> +// Switch unusual (Error **) parameter names to errp
>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
>>
>> Please put your rule comments right before the rule, i.e. before the
>> @-line introducing metavariable declarations, not after.  Same
>> elsewhere.
>>
>>> +identifier rule0.fn;
>>> +identifier rule0.ERRP != errp;
>>> +@@
>>> +
>>> + fn(...,
>>> +-   Error **ERRP
>>> ++   Error **errp
>>> +    ,...)
>>> + {
>>> +     <...
>>> +-    ERRP
>>> ++    errp
>>> +     ...>
>>> + }
>>
>> This normalizes errp parameter naming.  It matches exactly when rule0
>> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
>> is unusual.  Good.
>>
>>> +
>>> +@rule1@
>>> +// We want to patch error propagation in functions regardless of
>>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>>> +// applying rule0, hence this one does not inherit from it.
>>
>> I'm not sure I get this comment.  Let's see what the rule does.
>>
>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(..., Error **errp, ...)
>>> + {
>>> +     <...
>>> +-    Error *local_err = NULL;
>>> +     ...>
>>> + }
>>
>> rule1 matches like rule0, except the Error ** parameter match is
>> tightened from any C identifier to the C identifier errp, and the
>> function body match tightened from "either use
>> error_prepend/error_append_hint or which use local_err+error_propagate
>> to check errors" to just the latter.
>>
>> I figure tightening the Error ** parameter match has no effect, because
>> we already normalized the parameter name.
>>
>> So rule1 deletes variable local_err where rule0 applied.  Correct?
>
> The difference with rule0 is that rule0 contains
>  "when != ERRP_AUTO_PROPAGATE()", so rule0 is not applied where
> we already have macro invocation.

Ah, I missed the when clause.

> This is why we can't inherit from rule0.
>
> No we believe that we have ERRP_AUTO_PROPAGATE invocation in all
> corresponding places (added by rule0 or before script run) and want to
> update all usage of local_err objects.

Let's see whether I got it:

* The first rule (rule0) adds ERRP_AUTO_PROPAGATE() to all functions
  that take an Error ** parameter, and either pass it error_prepend() or
  error_append_hint(), or use local_err, and don't have
  ERRP_AUTO_PROPAGATE() already, except it skips the ones named
  error_append_FOO_hint().  Uff.

  The "use local_err" part is an approximation of "use local_err +
  error_propagate()".

  The "except for the ones named error_append_FOO_hint()" part is an
  approximation of "except for the ones taking an Error *const *
  parameter".

  ERRP_AUTO_PROPAGATE() requires the Error ** parameter to be named
  @errp, which need not be the case.  The next rule fixes it up:

* The second rule ensures the parameter is named @errp wherever the
  first rule applied, renaming if necessary.

  Correct?

  Incorrect transformation followed by fixup is not ideal, because it
  can trip up reviewers.  But ideal is too expensive; this is good
  enough.

* The third rule (rule1) ensures functions that take an Error **errp
  parameter don't declare local_err, except it skips the ones named
  error_append_FOO_hint().

  In isolation, this rule makes no sense.  To make sense of it, we need
  context:

  * Subsequent rules remove all uses of @errp from any function where
    rule1 matches.

  * Preceding rules ensure any function where rule1 matches has
    ERRP_AUTO_PROPAGATE().

  Correct?

  The need for this much context is hard on reviewers.  Good enough for
  transforming the tree now, but I'd hate having to make sense of this
  again in six months.

>>> +
>>> +@@
>>> +// Handle pattern with goto, otherwise we'll finish up
>>> +// with labels at function end which will not compile.
>>> +identifier rule1.fn, rule1.local_err;
>>> +identifier OUT;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +-    goto OUT;
>>> ++    return;
>>> +     ...>
>>> +- OUT:
>>> +-    error_propagate(errp, local_err);
>>> + }
>>
>> This is one special case of error_propagate() deletion.  It additionally
>> gets rid of a goto we no longer want.  For the general case, see below.
>>
>> The rule applies only where rule1 just deleted the variable.  Thus, the
>> two rules work in tandem.  Makes sense.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>
>> This rule also works in tandem with rule1.
>>
>>> +expression list args; // to reindent error_propagate_prepend
>>
>> What is the comment trying to tell me?
>
> Hmm, we can safely drop it. It's about the following:
>
> instead of
>
>  -    error_propagate_prepend(errp, local_err, args);
>  +    error_prepend(errp, args);
>
> we can use "...", like
>
>  - error_propagate_prepend(errp, local_err
>  + error_prepend(errp
>    , ...);
>
> but with metavar in use, coccinelle will correctly reindent the
> whole call, which looks a lot better.

Let's drop the comment.

>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    error_free(local_err);
>>> +-    local_err = NULL;
>>> ++    error_free_errp(errp);
>>
>> Reminder:
>>
>>      static inline void error_free_errp(Error **errp)
>>      {
>>          assert(errp && *errp);
>>          error_free(*errp);
>>          *errp = NULL;
>>      }
>>
>> Now let's examine the actual change.
>>
>> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
>> ensures it.
>>
>> The second half is new.  We now crash when we haven't set an error.  Why
>> is this safe?  Note that error_free(local_err) does nothing when
>> !local_err.
>
> Hmm. Looks like we should tighten this restriction, and follow error_free
> interface, which allows freeing unset errp.
>
>>
>> The zapping of the variable pointing to the Error just freed is
>> unchanged.
>>
>>> +|
>>> +-    error_free(local_err);
>>> ++    error_free_errp(errp);
>>
>> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
>> Needed, or else the automatic error_propagate() due to
>> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
>>
>>> +|
>>> +-    error_report_err(local_err);
>>> ++    error_report_errp(errp);
>>
>> The only difference to the previous case is that we also report the
>> error.
>>
>> The previous case has a buddy that additionally matches *errp = NULL.
>> Why not this one?
>
> Probably because no matches in code. But should be added here for
> better case coverage.

Either that or a comment pointing out what's missing, and why, namely
because the pattern doesn't exist in the tree.

>>
>>> +|
>>> +-    warn_report_err(local_err);
>>> ++    warn_report_errp(errp);
>>
>> Likewise.
>>
>> What about error_reportf_err(), warn_reportf_err()?
>>
>> Up to here, this rule transforms the various forms of error_free().
>> Next: error_propagate().
>>
>>> +|
>>> +-    error_propagate_prepend(errp, local_err, args);
>>> ++    error_prepend(errp, args);
>>> +|
>>> +-    error_propagate(errp, local_err);
>>
>> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
>> redundant.
>>
>> This is the general case of error_propagate() deletion.
>>
>> I'd put the plain error_propagate() first, variations second, like you
>> do with error_free().
>>
>> If neither of these two patterns match on a path from
>> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
>> where it wasn't before.  Does nothing when the local error is null
>> there.  Bug fix when it isn't: it's at least a memory leak, and quite
>> possibly worse.
>
> Hmm. How can it be memory leak after any of error_free variants?

Consider nfs_options_qdict_to_qapi() right before commit 54b7af4369a
fixed it:

    static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
                                                         Error **errp)
    {
        BlockdevOptionsNfs *opts = NULL;
        QObject *crumpled = NULL;
        Visitor *v;
        Error *local_err = NULL;

        crumpled = qdict_crumple(options, errp);
        if (crumpled == NULL) {
            return NULL;
        }

        v = qobject_input_visitor_new_keyval(crumpled);
        visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err);
        visit_free(v);
        qobject_unref(crumpled);

        if (local_err) {
            return NULL;
        }

        return opts;
    }

When visit_type_BlockdevOptionsNfs() fails, we return null without
setting an error.  We also leak the error we got from
visit_type_BlockdevOptionsNfs().

Commit 54b7af4369a fixed this:

    --- a/block/nfs.c
    +++ b/block/nfs.c
    @@ -570,6 +570,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *
    options,
         qobject_unref(crumpled);

         if (local_err) {
    +        error_propagate(errp, local_err);
             return NULL;
         }

If it was still broken, then your transformation would *also* fix it,
wouldn't it?

My point is: your transformation might fix actual bugs!

>> Identifying these bug fixes would be nice, but I don't have practical
>> ideas on how to do that.
>>
>> Can we explain this in the commit message?
>>
>>> +)
>>> +     ...>
>>> + }
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    &local_err
>>> ++    errp
>>> +|
>>> +-    local_err
>>> ++    *errp
>>> +)
>>> +     ...>
>>> + }
>>
>> Also in tandem with rule1, fixes up uses of local_err.  Good.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +- *errp != NULL
>>> ++ *errp
>>> +     ...>
>>> + }
>>
>> Still in tandem with rule1, normalizes style.  Good.
>>

25.02.2020 15:52, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 23.02.2020 11:55, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>>> does corresponding changes in code (look for details in
>>>> include/qapi/error.h)
>>>>
>>>> Usage example:
>>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>    --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>    blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>
>>>> CC: Eric Blake <eblake@redhat.com>
>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>> CC: Max Reitz <mreitz@redhat.com>
>>>> CC: Greg Kurz <groug@kaod.org>
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Anthony Perard <anthony.perard@citrix.com>
>>>> CC: Paul Durrant <paul@xen.org>
>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>>> CC: Laszlo Ersek <lersek@redhat.com>
>>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>>> CC: Stefan Berger <stefanb@linux.ibm.com>
>>>> CC: Markus Armbruster <armbru@redhat.com>
>>>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>>>> CC: qemu-block@nongnu.org
>>>> CC: xen-devel@lists.xenproject.org
>>>>
>>>>    include/qapi/error.h                          |   3 +
>>>>    scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>>    2 files changed, 161 insertions(+)
>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>
>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>> index b9452d4806..79f8e95214 100644
>>>> --- a/include/qapi/error.h
>>>> +++ b/include/qapi/error.h
>>>> @@ -141,6 +141,9 @@
>>>>     *         ...
>>>>     *     }
>>>>     *
>>>> + * For mass conversion use script
>>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>>> + *
>>>>     *
>>>>     * Receive and accumulate multiple errors (first one wins):
>>>>     *     Error *err = NULL, *local_err = NULL;
>>>
>>> Extra blank line.
>>>
>>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>>> new file mode 100644
>>>> index 0000000000..fb03c871cb
>>>> --- /dev/null
>>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>>> @@ -0,0 +1,158 @@
>>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>>> +//
>>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>>> +//
>>>> +// This program is free software; you can redistribute it and/or modify
>>>> +// it under the terms of the GNU General Public License as published by
>>>> +// the Free Software Foundation; either version 2 of the License, or
>>>> +// (at your option) any later version.
>>>> +//
>>>> +// This program is distributed in the hope that it will be useful,
>>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> +// GNU General Public License for more details.
>>>> +//
>>>> +// You should have received a copy of the GNU General Public License
>>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> +//
>>>> +// Usage example:
>>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>> +
>>>> +@rule0@
>>>> +// Add invocation to errp-functions where necessary
>>>> +// We should skip functions with "Error *const *errp"
>>>> +// parameter, but how to do it with coccinelle?
>>>> +// I don't know, so, I skip them by function name regex.
>>>> +// It's safe: if we did not skip some functions with
>>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>>> +// will fail to compile, because of const violation.
>>>
>>> Not skipping a function we should skip fails to compile.
>>>
>>> What about skipping a function we should not skip?
>>
>> Then it will not be updated.. Not good but I don't have better solution.
>> Still, I hope, function called *error_append_*_hint will not return error
>> through errp pointer.
> 
> Seems likely.  I just dislike inferring behavior from name patterns.
> 
> Ideally, we recognize the true exceptional pattern instead, i.e. the
> presence of const.  But figuring out how to make Coccinelle do that for
> us may be more trouble than it's worth.
> 
> Hmm...  Coccinelle matches the parameter even with const due to what it
> calls "isomorphism".  Can I disable it?  *Tinker* *tinker*
> 
> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
> index fb03c871cb..0c4414bff3 100644
> --- a/scripts/coccinelle/auto-propagated-errp.cocci
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -20,15 +20,11 @@
>   //  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>   //  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>   
> -@rule0@
> +@rule0 disable optional_qualifier@
>   // Add invocation to errp-functions where necessary
> -// We should skip functions with "Error *const *errp"
> -// parameter, but how to do it with coccinelle?
> -// I don't know, so, I skip them by function name regex.
> -// It's safe: if we did not skip some functions with
> -// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
> -// will fail to compile, because of const violation.
> -identifier fn !~ "error_append_.*_hint";
> +// Disable optional_qualifier to skip functions with "Error *const *errp"
> +// parameter,
> +identifier fn;
>   identifier local_err, ERRP;
>   @@
> 
> Could you verify this results in the same tree-wide change as your
> version?

Yes, no difference. Thanks!

> 
>>>> +identifier fn !~ "error_append_.*_hint";
>>>> +identifier local_err, ERRP;
>>>
>>> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
>>> don't.  Either is fine with me.  Mixing the two styles feels a bit
>>> confusing, though.
>>>
>>>> +@@
>>>> +
>>>> + fn(..., Error **ERRP, ...)
>>>> + {
>>>> ++   ERRP_AUTO_PROPAGATE();
>>>> +    <+...
>>>> +        when != ERRP_AUTO_PROPAGATE();
>>>> +(
>>>> +    error_append_hint(ERRP, ...);
>>>> +|
>>>> +    error_prepend(ERRP, ...);
>>>> +|
>>>> +    Error *local_err = NULL;
>>>> +)
>>>> +    ...+>
>>>> + }
>>>
>>> Misses error_vprepend().  Currently harmless, but as long as we commit
>>> the script, we better make it as robust as we reasonably can.
>>>
>>> The previous patch explains this Coccinelle script's intent:
>>>
>>>     To achieve these goals, later patches will add invocations
>>>     of this macro at the start of functions with either use
>>>     error_prepend/error_append_hint (solving 1) or which use
>>>     local_err+error_propagate to check errors, switching those
>>>     functions to use *errp instead (solving 2 and 3).
>>>
>>> This rule matches "use error_prepend/error_append_hint" directly.  It
>>> appears to use presence of a local Error * variable as proxy for "use
>>> local_err+error_propagate to check errors".  Hmm.
>>>
>>> We obviously have such a variable when we use "local_err+error_propagate
>>> to check errors".  But we could also have such variables without use of
>>> error_propagate().  In fact, error.h documents such use:
>>>
>>>    * Call a function and receive an error from it:
>>>    *     Error *err = NULL;
>>>    *     foo(arg, &err);
>>>    *     if (err) {
>>>    *         handle the error...
>>>    *     }
>>>
>>> where "handle the error" frees it.
>>>
>>> I figure such uses typically occur in functions without an Error **errp
>>> parameter.  This rule doesn't apply then.  But they could occur even in
>>> functions with such a parameter.  Consider:
>>>
>>>       void foo(Error **errp)
>>>       {
>>>           Error *err = NULL;
>>>
>>>           bar(&err);
>>>           if (err) {
>>>               error_free(err);
>>>               error_setg(errp, "completely different error");
>>>           }
>>>       }
>>>
>>> Reasonable enough when bar() gives us an error that's misleading in this
>>> context, isn't it?
>>>
>>> The script transforms it like this:
>>>
>>>       void foo(Error **errp)
>>>       {
>>>      -    Error *err = NULL;
>>>      +    ERRP_AUTO_PROPAGATE();
>>>
>>>      -    bar(&err);
>>>      -    if (err) {
>>>      -        error_free(err);
>>>      +    bar(errp);
>>>      +    if (*errp) {
>>>      +        error_free_errp(errp);
>>>               error_setg(errp, "completely different error");
>>>           }
>>>       }
>>>
>>> Unwanted.
>>
>> What is the problem with it? Updated code just use "new usual notation"
>> for handling error of sub-calls in function which reports errors through
>> errp pointer.
> 
> error.h's big comment asks for use of ERRP_AUTO_PROPAGATE() to "Receive
> an error and pass it on to the caller".  We're not doing that here.  We
> "Call a function and receive an error from it", then "Handle an error
> without reporting it".
> 
> The updated code works anyway, but it's needlessly complicated.

OK, will try.

> 
>>> Now, if this script applied in just a few dozen places, we could rely on
>>> eyeballing its output to catch unwanted transformations.  Since it
>>> applies in so many more, I don't feel comfortable relying on reviewer
>>> eyeballs.
>>>
>>> Can we make rule0 directly match error_propagate(errp, local_err)
>>> somehow?
>>
>> I think it is possible, still I'm not sure we need it.
> 
> We don't need it in the sense of "must have to avoid a buggy
> transformation".  It's more like "I'd like to have it to stay close to
> the documented usage of ERRP_AUTO_PROPAGATE(), and to avoid complicating
> cases like the one above".
> 
>>> Another observation: the rule does not match error_reportf_err() and
>>> warn_reportf_err().  These combine error_prepend(),
>>> error_report()/warn_report() and error_free(), for convenience.  Don't
>>> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
>>> users?
>>
>> Right. These functions want to add information, which will not work
>> for error_fatal without wrapping.
> 
> A simple improvement, I hope.
> 
>>>> +
>>>> +@@
>>>> +// Switch unusual (Error **) parameter names to errp
>>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
>>>
>>> Please put your rule comments right before the rule, i.e. before the
>>> @-line introducing metavariable declarations, not after.  Same
>>> elsewhere.
>>>
>>>> +identifier rule0.fn;
>>>> +identifier rule0.ERRP != errp;
>>>> +@@
>>>> +
>>>> + fn(...,
>>>> +-   Error **ERRP
>>>> ++   Error **errp
>>>> +    ,...)
>>>> + {
>>>> +     <...
>>>> +-    ERRP
>>>> ++    errp
>>>> +     ...>
>>>> + }
>>>
>>> This normalizes errp parameter naming.  It matches exactly when rule0
>>> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
>>> is unusual.  Good.
>>>
>>>> +
>>>> +@rule1@
>>>> +// We want to patch error propagation in functions regardless of
>>>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>>>> +// applying rule0, hence this one does not inherit from it.
>>>
>>> I'm not sure I get this comment.  Let's see what the rule does.
>>>
>>>> +identifier fn !~ "error_append_.*_hint";
>>>> +identifier local_err;
>>>> +symbol errp;
>>>> +@@
>>>> +
>>>> + fn(..., Error **errp, ...)
>>>> + {
>>>> +     <...
>>>> +-    Error *local_err = NULL;
>>>> +     ...>
>>>> + }
>>>
>>> rule1 matches like rule0, except the Error ** parameter match is
>>> tightened from any C identifier to the C identifier errp, and the
>>> function body match tightened from "either use
>>> error_prepend/error_append_hint or which use local_err+error_propagate
>>> to check errors" to just the latter.
>>>
>>> I figure tightening the Error ** parameter match has no effect, because
>>> we already normalized the parameter name.
>>>
>>> So rule1 deletes variable local_err where rule0 applied.  Correct?
>>
>> The difference with rule0 is that rule0 contains
>>   "when != ERRP_AUTO_PROPAGATE()", so rule0 is not applied where
>> we already have macro invocation.
> 
> Ah, I missed the when clause.
> 
>> This is why we can't inherit from rule0.
>>
>> No we believe that we have ERRP_AUTO_PROPAGATE invocation in all
>> corresponding places (added by rule0 or before script run) and want to
>> update all usage of local_err objects.
> 
> Let's see whether I got it:
> 
> * The first rule (rule0) adds ERRP_AUTO_PROPAGATE() to all functions
>    that take an Error ** parameter, and either pass it error_prepend() or
>    error_append_hint(), or use local_err, and don't have
>    ERRP_AUTO_PROPAGATE() already, except it skips the ones named
>    error_append_FOO_hint().  Uff.
> 
>    The "use local_err" part is an approximation of "use local_err +
>    error_propagate()".
> 
>    The "except for the ones named error_append_FOO_hint()" part is an
>    approximation of "except for the ones taking an Error *const *
>    parameter".
> 
>    ERRP_AUTO_PROPAGATE() requires the Error ** parameter to be named
>    @errp, which need not be the case.  The next rule fixes it up:
> 
> * The second rule ensures the parameter is named @errp wherever the
>    first rule applied, renaming if necessary.
> 
>    Correct?
> 
>    Incorrect transformation followed by fixup is not ideal, because it
>    can trip up reviewers.  But ideal is too expensive; this is good
>    enough.
> 
> * The third rule (rule1) ensures functions that take an Error **errp
>    parameter don't declare local_err, except it skips the ones named
>    error_append_FOO_hint().
> 
>    In isolation, this rule makes no sense.  To make sense of it, we need
>    context:
> 
>    * Subsequent rules remove all uses of @errp from any function where

of local_err

>      rule1 matches.
> 
>    * Preceding rules ensure any function where rule1 matches has
>      ERRP_AUTO_PROPAGATE().
> 
>    Correct?

Oh, yes, everything is correct.

> 
>    The need for this much context is hard on reviewers.  Good enough for
>    transforming the tree now, but I'd hate having to make sense of this
>    again in six months.

Ohh, yes. Far from good design. I can try to reorder chunks a bit.

> 
>>>> +
>>>> +@@
>>>> +// Handle pattern with goto, otherwise we'll finish up
>>>> +// with labels at function end which will not compile.
>>>> +identifier rule1.fn, rule1.local_err;
>>>> +identifier OUT;
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +-    goto OUT;
>>>> ++    return;
>>>> +     ...>
>>>> +- OUT:
>>>> +-    error_propagate(errp, local_err);
>>>> + }
>>>
>>> This is one special case of error_propagate() deletion.  It additionally
>>> gets rid of a goto we no longer want.  For the general case, see below.
>>>
>>> The rule applies only where rule1 just deleted the variable.  Thus, the
>>> two rules work in tandem.  Makes sense.
>>>
>>>> +
>>>> +@@
>>>> +identifier rule1.fn, rule1.local_err;
>>>
>>> This rule also works in tandem with rule1.
>>>
>>>> +expression list args; // to reindent error_propagate_prepend
>>>
>>> What is the comment trying to tell me?
>>
>> Hmm, we can safely drop it. It's about the following:
>>
>> instead of
>>
>>   -    error_propagate_prepend(errp, local_err, args);
>>   +    error_prepend(errp, args);
>>
>> we can use "...", like
>>
>>   - error_propagate_prepend(errp, local_err
>>   + error_prepend(errp
>>     , ...);
>>
>> but with metavar in use, coccinelle will correctly reindent the
>> whole call, which looks a lot better.
> 
> Let's drop the comment.
> 
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +(
>>>> +-    error_free(local_err);
>>>> +-    local_err = NULL;
>>>> ++    error_free_errp(errp);
>>>
>>> Reminder:
>>>
>>>       static inline void error_free_errp(Error **errp)
>>>       {
>>>           assert(errp && *errp);
>>>           error_free(*errp);
>>>           *errp = NULL;
>>>       }
>>>
>>> Now let's examine the actual change.
>>>
>>> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
>>> ensures it.
>>>
>>> The second half is new.  We now crash when we haven't set an error.  Why
>>> is this safe?  Note that error_free(local_err) does nothing when
>>> !local_err.
>>
>> Hmm. Looks like we should tighten this restriction, and follow error_free
>> interface, which allows freeing unset errp.
>>
>>>
>>> The zapping of the variable pointing to the Error just freed is
>>> unchanged.
>>>
>>>> +|
>>>> +-    error_free(local_err);
>>>> ++    error_free_errp(errp);
>>>
>>> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
>>> Needed, or else the automatic error_propagate() due to
>>> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
>>>
>>>> +|
>>>> +-    error_report_err(local_err);
>>>> ++    error_report_errp(errp);
>>>
>>> The only difference to the previous case is that we also report the
>>> error.
>>>
>>> The previous case has a buddy that additionally matches *errp = NULL.
>>> Why not this one?
>>
>> Probably because no matches in code. But should be added here for
>> better case coverage.
> 
> Either that or a comment pointing out what's missing, and why, namely
> because the pattern doesn't exist in the tree.
> 
>>>
>>>> +|
>>>> +-    warn_report_err(local_err);
>>>> ++    warn_report_errp(errp);
>>>
>>> Likewise.
>>>
>>> What about error_reportf_err(), warn_reportf_err()?
>>>
>>> Up to here, this rule transforms the various forms of error_free().
>>> Next: error_propagate().
>>>
>>>> +|
>>>> +-    error_propagate_prepend(errp, local_err, args);
>>>> ++    error_prepend(errp, args);
>>>> +|
>>>> +-    error_propagate(errp, local_err);
>>>
>>> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
>>> redundant.
>>>
>>> This is the general case of error_propagate() deletion.
>>>
>>> I'd put the plain error_propagate() first, variations second, like you
>>> do with error_free().
>>>
>>> If neither of these two patterns match on a path from
>>> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
>>> where it wasn't before.  Does nothing when the local error is null
>>> there.  Bug fix when it isn't: it's at least a memory leak, and quite
>>> possibly worse.
>>
>> Hmm. How can it be memory leak after any of error_free variants?
> 
> Consider nfs_options_qdict_to_qapi() right before commit 54b7af4369a
> fixed it:
> 
>      static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
>                                                           Error **errp)
>      {
>          BlockdevOptionsNfs *opts = NULL;
>          QObject *crumpled = NULL;
>          Visitor *v;
>          Error *local_err = NULL;
> 
>          crumpled = qdict_crumple(options, errp);
>          if (crumpled == NULL) {
>              return NULL;
>          }
> 
>          v = qobject_input_visitor_new_keyval(crumpled);
>          visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err);
>          visit_free(v);
>          qobject_unref(crumpled);
> 
>          if (local_err) {
>              return NULL;
>          }
> 
>          return opts;
>      }
> 
> When visit_type_BlockdevOptionsNfs() fails, we return null without
> setting an error.  We also leak the error we got from
> visit_type_BlockdevOptionsNfs().
> 
> Commit 54b7af4369a fixed this:
> 
>      --- a/block/nfs.c
>      +++ b/block/nfs.c
>      @@ -570,6 +570,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *
>      options,
>           qobject_unref(crumpled);
> 
>           if (local_err) {
>      +        error_propagate(errp, local_err);
>               return NULL;
>           }
> 
> If it was still broken, then your transformation would *also* fix it,
> wouldn't it?

Ah, yes. You mean addition of the macro invocation, I thought about transforming
error_free variants to error_free_errp variants.

> 
> My point is: your transformation might fix actual bugs!
> 
>>> Identifying these bug fixes would be nice, but I don't have practical
>>> ideas on how to do that.
>>>
>>> Can we explain this in the commit message?
>>>
>>>> +)
>>>> +     ...>
>>>> + }
>>>> +
>>>> +@@
>>>> +identifier rule1.fn, rule1.local_err;
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +(
>>>> +-    &local_err
>>>> ++    errp
>>>> +|
>>>> +-    local_err
>>>> ++    *errp
>>>> +)
>>>> +     ...>
>>>> + }
>>>
>>> Also in tandem with rule1, fixes up uses of local_err.  Good.
>>>
>>>> +
>>>> +@@
>>>> +identifier rule1.fn;
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +- *errp != NULL
>>>> ++ *errp
>>>> +     ...>
>>>> + }
>>>
>>> Still in tandem with rule1, normalizes style.  Good.
>>>
>

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 25.02.2020 15:52, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 23.02.2020 11:55, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>>>> does corresponding changes in code (look for details in
>>>>> include/qapi/error.h)
>>>>>
>>>>> Usage example:
>>>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>>    --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>>    blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>
>>>>> CC: Eric Blake <eblake@redhat.com>
>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>>> CC: Max Reitz <mreitz@redhat.com>
>>>>> CC: Greg Kurz <groug@kaod.org>
>>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>>> CC: Anthony Perard <anthony.perard@citrix.com>
>>>>> CC: Paul Durrant <paul@xen.org>
>>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>>>> CC: Laszlo Ersek <lersek@redhat.com>
>>>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>>>> CC: Stefan Berger <stefanb@linux.ibm.com>
>>>>> CC: Markus Armbruster <armbru@redhat.com>
>>>>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>>>>> CC: qemu-block@nongnu.org
>>>>> CC: xen-devel@lists.xenproject.org
>>>>>
>>>>>    include/qapi/error.h                          |   3 +
>>>>>    scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>>>    2 files changed, 161 insertions(+)
>>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>>
>>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>>> index b9452d4806..79f8e95214 100644
>>>>> --- a/include/qapi/error.h
>>>>> +++ b/include/qapi/error.h
>>>>> @@ -141,6 +141,9 @@
>>>>>     *         ...
>>>>>     *     }
>>>>>     *
>>>>> + * For mass conversion use script
>>>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>>>> + *
>>>>>     *
>>>>>     * Receive and accumulate multiple errors (first one wins):
>>>>>     *     Error *err = NULL, *local_err = NULL;
>>>>
>>>> Extra blank line.
>>>>
>>>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>>>> new file mode 100644
>>>>> index 0000000000..fb03c871cb
>>>>> --- /dev/null
>>>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>>>> @@ -0,0 +1,158 @@
>>>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>>>> +//
>>>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>>>> +//
>>>>> +// This program is free software; you can redistribute it and/or modify
>>>>> +// it under the terms of the GNU General Public License as published by
>>>>> +// the Free Software Foundation; either version 2 of the License, or
>>>>> +// (at your option) any later version.
>>>>> +//
>>>>> +// This program is distributed in the hope that it will be useful,
>>>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> +// GNU General Public License for more details.
>>>>> +//
>>>>> +// You should have received a copy of the GNU General Public License
>>>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>> +//
>>>>> +// Usage example:
>>>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>> +
>>>>> +@rule0@
>>>>> +// Add invocation to errp-functions where necessary
>>>>> +// We should skip functions with "Error *const *errp"
>>>>> +// parameter, but how to do it with coccinelle?
>>>>> +// I don't know, so, I skip them by function name regex.
>>>>> +// It's safe: if we did not skip some functions with
>>>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>>>> +// will fail to compile, because of const violation.
>>>>
>>>> Not skipping a function we should skip fails to compile.
>>>>
>>>> What about skipping a function we should not skip?
>>>
>>> Then it will not be updated.. Not good but I don't have better solution.
>>> Still, I hope, function called *error_append_*_hint will not return error
>>> through errp pointer.
>>
>> Seems likely.  I just dislike inferring behavior from name patterns.
>>
>> Ideally, we recognize the true exceptional pattern instead, i.e. the
>> presence of const.  But figuring out how to make Coccinelle do that for
>> us may be more trouble than it's worth.
>>
>> Hmm...  Coccinelle matches the parameter even with const due to what it
>> calls "isomorphism".  Can I disable it?  *Tinker* *tinker*
>>
>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>> index fb03c871cb..0c4414bff3 100644
>> --- a/scripts/coccinelle/auto-propagated-errp.cocci
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>> @@ -20,15 +20,11 @@
>>   //  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>   //  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>   -@rule0@
>> +@rule0 disable optional_qualifier@
>>   // Add invocation to errp-functions where necessary
>> -// We should skip functions with "Error *const *errp"
>> -// parameter, but how to do it with coccinelle?
>> -// I don't know, so, I skip them by function name regex.
>> -// It's safe: if we did not skip some functions with
>> -// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>> -// will fail to compile, because of const violation.
>> -identifier fn !~ "error_append_.*_hint";
>> +// Disable optional_qualifier to skip functions with "Error *const *errp"
>> +// parameter,
>> +identifier fn;
>>   identifier local_err, ERRP;
>>   @@
>>
>> Could you verify this results in the same tree-wide change as your
>> version?
>
> Yes, no difference. Thanks!

Excellent!

[...]
>> Let's see whether I got it:
>>
>> * The first rule (rule0) adds ERRP_AUTO_PROPAGATE() to all functions
>>    that take an Error ** parameter, and either pass it error_prepend() or
>>    error_append_hint(), or use local_err, and don't have
>>    ERRP_AUTO_PROPAGATE() already, except it skips the ones named
>>    error_append_FOO_hint().  Uff.
>>
>>    The "use local_err" part is an approximation of "use local_err +
>>    error_propagate()".
>>
>>    The "except for the ones named error_append_FOO_hint()" part is an
>>    approximation of "except for the ones taking an Error *const *
>>    parameter".
>>
>>    ERRP_AUTO_PROPAGATE() requires the Error ** parameter to be named
>>    @errp, which need not be the case.  The next rule fixes it up:
>>
>> * The second rule ensures the parameter is named @errp wherever the
>>    first rule applied, renaming if necessary.
>>
>>    Correct?
>>
>>    Incorrect transformation followed by fixup is not ideal, because it
>>    can trip up reviewers.  But ideal is too expensive; this is good
>>    enough.
>>
>> * The third rule (rule1) ensures functions that take an Error **errp
>>    parameter don't declare local_err, except it skips the ones named
>>    error_append_FOO_hint().
>>
>>    In isolation, this rule makes no sense.  To make sense of it, we need
>>    context:
>>
>>    * Subsequent rules remove all uses of @errp from any function where
>
> of local_err
>
>>      rule1 matches.
>>
>>    * Preceding rules ensure any function where rule1 matches has
>>      ERRP_AUTO_PROPAGATE().
>>
>>    Correct?
>
> Oh, yes, everything is correct.

Thank you!

>>
>>    The need for this much context is hard on reviewers.  Good enough for
>>    transforming the tree now, but I'd hate having to make sense of this
>>    again in six months.
>
> Ohh, yes. Far from good design. I can try to reorder chunks a bit.

Please don't spend too much effort on it.  The script is primarily for
helping us convert the whole tree within a short time span.  We may also
use it later to convert instances of the old pattern that have crept
back.  We hopefully won't have to change the script then.  Readability
is not as important as it is for code we expect to be read again and
again over a long time.

[...]

23.02.2020 11:55, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>> does corresponding changes in code (look for details in
>> include/qapi/error.h)
>>
>> Usage example:
>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> CC: Greg Kurz <groug@kaod.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Anthony Perard <anthony.perard@citrix.com>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>> CC: Laszlo Ersek <lersek@redhat.com>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> CC: Stefan Berger <stefanb@linux.ibm.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> CC: qemu-block@nongnu.org
>> CC: xen-devel@lists.xenproject.org
>>
>>   include/qapi/error.h                          |   3 +
>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>   2 files changed, 161 insertions(+)
>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index b9452d4806..79f8e95214 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -141,6 +141,9 @@
>>    *         ...
>>    *     }
>>    *
>> + * For mass conversion use script
>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>> + *
>>    *
>>    * Receive and accumulate multiple errors (first one wins):
>>    *     Error *err = NULL, *local_err = NULL;
> 
> Extra blank line.
> 
>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>> new file mode 100644
>> index 0000000000..fb03c871cb
>> --- /dev/null
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>> @@ -0,0 +1,158 @@
>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>> +//
>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>> +//
>> +// This program is free software; you can redistribute it and/or modify
>> +// it under the terms of the GNU General Public License as published by
>> +// the Free Software Foundation; either version 2 of the License, or
>> +// (at your option) any later version.
>> +//
>> +// This program is distributed in the hope that it will be useful,
>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +// GNU General Public License for more details.
>> +//
>> +// You should have received a copy of the GNU General Public License
>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +//
>> +// Usage example:
>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>> +
>> +@rule0@
>> +// Add invocation to errp-functions where necessary
>> +// We should skip functions with "Error *const *errp"
>> +// parameter, but how to do it with coccinelle?
>> +// I don't know, so, I skip them by function name regex.
>> +// It's safe: if we did not skip some functions with
>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>> +// will fail to compile, because of const violation.
> 
> Not skipping a function we should skip fails to compile.
> 
> What about skipping a function we should not skip?
> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err, ERRP;
> 
> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
> don't.  Either is fine with me.  Mixing the two styles feels a bit
> confusing, though.
> 
>> +@@
>> +
>> + fn(..., Error **ERRP, ...)
>> + {
>> ++   ERRP_AUTO_PROPAGATE();
>> +    <+...
>> +        when != ERRP_AUTO_PROPAGATE();
>> +(
>> +    error_append_hint(ERRP, ...);
>> +|
>> +    error_prepend(ERRP, ...);
>> +|
>> +    Error *local_err = NULL;
>> +)
>> +    ...+>
>> + }
> 
> Misses error_vprepend().  Currently harmless, but as long as we commit
> the script, we better make it as robust as we reasonably can.
> 
> The previous patch explains this Coccinelle script's intent:
> 
>    To achieve these goals, later patches will add invocations
>    of this macro at the start of functions with either use
>    error_prepend/error_append_hint (solving 1) or which use
>    local_err+error_propagate to check errors, switching those
>    functions to use *errp instead (solving 2 and 3).
> 
> This rule matches "use error_prepend/error_append_hint" directly.  It
> appears to use presence of a local Error * variable as proxy for "use
> local_err+error_propagate to check errors".  Hmm.
> 
> We obviously have such a variable when we use "local_err+error_propagate
> to check errors".  But we could also have such variables without use of
> error_propagate().  In fact, error.h documents such use:
> 
>   * Call a function and receive an error from it:
>   *     Error *err = NULL;
>   *     foo(arg, &err);
>   *     if (err) {
>   *         handle the error...
>   *     }
> 
> where "handle the error" frees it.
> 
> I figure such uses typically occur in functions without an Error **errp
> parameter.  This rule doesn't apply then.  But they could occur even in
> functions with such a parameter.  Consider:
> 
>      void foo(Error **errp)
>      {
>          Error *err = NULL;
> 
>          bar(&err);
>          if (err) {
>              error_free(err);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Reasonable enough when bar() gives us an error that's misleading in this
> context, isn't it?
> 
> The script transforms it like this:
> 
>      void foo(Error **errp)
>      {
>     -    Error *err = NULL;
>     +    ERRP_AUTO_PROPAGATE();
> 
>     -    bar(&err);
>     -    if (err) {
>     -        error_free(err);
>     +    bar(errp);
>     +    if (*errp) {
>     +        error_free_errp(errp);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Unwanted.
> 
> Now, if this script applied in just a few dozen places, we could rely on
> eyeballing its output to catch unwanted transformations.  Since it
> applies in so many more, I don't feel comfortable relying on reviewer
> eyeballs.
> 
> Can we make rule0 directly match error_propagate(errp, local_err)
> somehow?
> 
> Another observation: the rule does not match error_reportf_err() and
> warn_reportf_err().

They are unrelated, as they take Error* argument, not Error**

> These combine error_prepend(),
> error_report()/warn_report() and error_free(), for convenience.  Don't
> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
> users?
> 
>> +
>> +@@
>> +// Switch unusual (Error **) parameter names to errp
>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
> 
> Please put your rule comments right before the rule, i.e. before the
> @-line introducing metavariable declarations, not after.  Same
> elsewhere.
> 
>> +identifier rule0.fn;
>> +identifier rule0.ERRP != errp;
>> +@@
>> +
>> + fn(...,
>> +-   Error **ERRP
>> ++   Error **errp
>> +    ,...)
>> + {
>> +     <...
>> +-    ERRP
>> ++    errp
>> +     ...>
>> + }
> 
> This normalizes errp parameter naming.  It matches exactly when rule0
> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
> is unusual.  Good.
> 
>> +
>> +@rule1@
>> +// We want to patch error propagation in functions regardless of
>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>> +// applying rule0, hence this one does not inherit from it.
> 
> I'm not sure I get this comment.  Let's see what the rule does.
> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err;
>> +symbol errp;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> +     <...
>> +-    Error *local_err = NULL;
>> +     ...>
>> + }
> 
> rule1 matches like rule0, except the Error ** parameter match is
> tightened from any C identifier to the C identifier errp, and the
> function body match tightened from "either use
> error_prepend/error_append_hint or which use local_err+error_propagate
> to check errors" to just the latter.
> 
> I figure tightening the Error ** parameter match has no effect, because
> we already normalized the parameter name.
> 
> So rule1 deletes variable local_err where rule0 applied.  Correct?
> 
>> +
>> +@@
>> +// Handle pattern with goto, otherwise we'll finish up
>> +// with labels at function end which will not compile.
>> +identifier rule1.fn, rule1.local_err;
>> +identifier OUT;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +-    goto OUT;
>> ++    return;
>> +     ...>
>> +- OUT:
>> +-    error_propagate(errp, local_err);
>> + }
> 
> This is one special case of error_propagate() deletion.  It additionally
> gets rid of a goto we no longer want.  For the general case, see below.
> 
> The rule applies only where rule1 just deleted the variable.  Thus, the
> two rules work in tandem.  Makes sense.
> 
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
> 
> This rule also works in tandem with rule1.
> 
>> +expression list args; // to reindent error_propagate_prepend
> 
> What is the comment trying to tell me?
> 
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    error_free(local_err);
>> +-    local_err = NULL;
>> ++    error_free_errp(errp);
> 
> Reminder:
> 
>      static inline void error_free_errp(Error **errp)
>      {
>          assert(errp && *errp);
>          error_free(*errp);
>          *errp = NULL;
>      }
> 
> Now let's examine the actual change.
> 
> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
> ensures it.
> 
> The second half is new.  We now crash when we haven't set an error.  Why
> is this safe?  Note that error_free(local_err) does nothing when
> !local_err.
> 
> The zapping of the variable pointing to the Error just freed is
> unchanged.
> 
>> +|
>> +-    error_free(local_err);
>> ++    error_free_errp(errp);
> 
> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
> Needed, or else the automatic error_propagate() due to
> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
> 
>> +|
>> +-    error_report_err(local_err);
>> ++    error_report_errp(errp);
> 
> The only difference to the previous case is that we also report the
> error.
> 
> The previous case has a buddy that additionally matches *errp = NULL.
> Why not this one?
> 
>> +|
>> +-    warn_report_err(local_err);
>> ++    warn_report_errp(errp);
> 
> Likewise.
> 
> What about error_reportf_err(), warn_reportf_err()?
> 
> Up to here, this rule transforms the various forms of error_free().
> Next: error_propagate().
> 
>> +|
>> +-    error_propagate_prepend(errp, local_err, args);
>> ++    error_prepend(errp, args);
>> +|
>> +-    error_propagate(errp, local_err);
> 
> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
> redundant.
> 
> This is the general case of error_propagate() deletion.
> 
> I'd put the plain error_propagate() first, variations second, like you
> do with error_free().
> 
> If neither of these two patterns match on a path from
> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
> where it wasn't before.  Does nothing when the local error is null
> there.  Bug fix when it isn't: it's at least a memory leak, and quite
> possibly worse.
> 
> Identifying these bug fixes would be nice, but I don't have practical
> ideas on how to do that.
> 
> Can we explain this in the commit message?
> 
>> +)
>> +     ...>
>> + }
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    &local_err
>> ++    errp
>> +|
>> +-    local_err
>> ++    *errp
>> +)
>> +     ...>
>> + }
> 
> Also in tandem with rule1, fixes up uses of local_err.  Good.
> 
>> +
>> +@@
>> +identifier rule1.fn;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +- *errp != NULL
>> ++ *errp
>> +     ...>
>> + }
> 
> Still in tandem with rule1, normalizes style.  Good.
>

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 23.02.2020 11:55, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>> does corresponding changes in code (look for details in
>>> include/qapi/error.h)
>>>
>>> Usage example:
>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>
>>> CC: Eric Blake <eblake@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Max Reitz <mreitz@redhat.com>
>>> CC: Greg Kurz <groug@kaod.org>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Anthony Perard <anthony.perard@citrix.com>
>>> CC: Paul Durrant <paul@xen.org>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>> CC: Laszlo Ersek <lersek@redhat.com>
>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>> CC: Stefan Berger <stefanb@linux.ibm.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> CC: qemu-block@nongnu.org
>>> CC: xen-devel@lists.xenproject.org
>>>
>>>   include/qapi/error.h                          |   3 +
>>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>   2 files changed, 161 insertions(+)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index b9452d4806..79f8e95214 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -141,6 +141,9 @@
>>>    *         ...
>>>    *     }
>>>    *
>>> + * For mass conversion use script
>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>> + *
>>>    *
>>>    * Receive and accumulate multiple errors (first one wins):
>>>    *     Error *err = NULL, *local_err = NULL;
>>
>> Extra blank line.
>>
>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>> new file mode 100644
>>> index 0000000000..fb03c871cb
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>> @@ -0,0 +1,158 @@
>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>> +//
>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>> +//
>>> +// This program is free software; you can redistribute it and/or modify
>>> +// it under the terms of the GNU General Public License as published by
>>> +// the Free Software Foundation; either version 2 of the License, or
>>> +// (at your option) any later version.
>>> +//
>>> +// This program is distributed in the hope that it will be useful,
>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +// GNU General Public License for more details.
>>> +//
>>> +// You should have received a copy of the GNU General Public License
>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +//
>>> +// Usage example:
>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>> +
>>> +@rule0@
>>> +// Add invocation to errp-functions where necessary
>>> +// We should skip functions with "Error *const *errp"
>>> +// parameter, but how to do it with coccinelle?
>>> +// I don't know, so, I skip them by function name regex.
>>> +// It's safe: if we did not skip some functions with
>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>> +// will fail to compile, because of const violation.
>>
>> Not skipping a function we should skip fails to compile.
>>
>> What about skipping a function we should not skip?
>>
>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err, ERRP;
>>
>> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
>> don't.  Either is fine with me.  Mixing the two styles feels a bit
>> confusing, though.
>>
>>> +@@
>>> +
>>> + fn(..., Error **ERRP, ...)
>>> + {
>>> ++   ERRP_AUTO_PROPAGATE();
>>> +    <+...
>>> +        when != ERRP_AUTO_PROPAGATE();
>>> +(
>>> +    error_append_hint(ERRP, ...);
>>> +|
>>> +    error_prepend(ERRP, ...);
>>> +|
>>> +    Error *local_err = NULL;
>>> +)
>>> +    ...+>
>>> + }
>>
>> Misses error_vprepend().  Currently harmless, but as long as we commit
>> the script, we better make it as robust as we reasonably can.
>>
>> The previous patch explains this Coccinelle script's intent:
>>
>>    To achieve these goals, later patches will add invocations
>>    of this macro at the start of functions with either use
>>    error_prepend/error_append_hint (solving 1) or which use
>>    local_err+error_propagate to check errors, switching those
>>    functions to use *errp instead (solving 2 and 3).
>>
>> This rule matches "use error_prepend/error_append_hint" directly.  It
>> appears to use presence of a local Error * variable as proxy for "use
>> local_err+error_propagate to check errors".  Hmm.
>>
>> We obviously have such a variable when we use "local_err+error_propagate
>> to check errors".  But we could also have such variables without use of
>> error_propagate().  In fact, error.h documents such use:
>>
>>   * Call a function and receive an error from it:
>>   *     Error *err = NULL;
>>   *     foo(arg, &err);
>>   *     if (err) {
>>   *         handle the error...
>>   *     }
>>
>> where "handle the error" frees it.
>>
>> I figure such uses typically occur in functions without an Error **errp
>> parameter.  This rule doesn't apply then.  But they could occur even in
>> functions with such a parameter.  Consider:
>>
>>      void foo(Error **errp)
>>      {
>>          Error *err = NULL;
>>
>>          bar(&err);
>>          if (err) {
>>              error_free(err);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Reasonable enough when bar() gives us an error that's misleading in this
>> context, isn't it?
>>
>> The script transforms it like this:
>>
>>      void foo(Error **errp)
>>      {
>>     -    Error *err = NULL;
>>     +    ERRP_AUTO_PROPAGATE();
>>
>>     -    bar(&err);
>>     -    if (err) {
>>     -        error_free(err);
>>     +    bar(errp);
>>     +    if (*errp) {
>>     +        error_free_errp(errp);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Unwanted.
>>
>> Now, if this script applied in just a few dozen places, we could rely on
>> eyeballing its output to catch unwanted transformations.  Since it
>> applies in so many more, I don't feel comfortable relying on reviewer
>> eyeballs.
>>
>> Can we make rule0 directly match error_propagate(errp, local_err)
>> somehow?
>>
>> Another observation: the rule does not match error_reportf_err() and
>> warn_reportf_err().
>
> They are unrelated, as they take Error* argument, not Error**
>
>> These combine error_prepend(),
>> error_report()/warn_report() and error_free(), for convenience.  Don't
>> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
>> users?

Right, we never pass *errp to error_reportf_err() and
warn_reportf_err(), so there's no need to wrap it.

But see below.

>>
>>> +
>>> +@@
>>> +// Switch unusual (Error **) parameter names to errp
>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
>>
>> Please put your rule comments right before the rule, i.e. before the
>> @-line introducing metavariable declarations, not after.  Same
>> elsewhere.
>>
>>> +identifier rule0.fn;
>>> +identifier rule0.ERRP != errp;
>>> +@@
>>> +
>>> + fn(...,
>>> +-   Error **ERRP
>>> ++   Error **errp
>>> +    ,...)
>>> + {
>>> +     <...
>>> +-    ERRP
>>> ++    errp
>>> +     ...>
>>> + }
>>
>> This normalizes errp parameter naming.  It matches exactly when rule0
>> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
>> is unusual.  Good.
>>
>>> +
>>> +@rule1@
>>> +// We want to patch error propagation in functions regardless of
>>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>>> +// applying rule0, hence this one does not inherit from it.
>>
>> I'm not sure I get this comment.  Let's see what the rule does.
>>
>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(..., Error **errp, ...)
>>> + {
>>> +     <...
>>> +-    Error *local_err = NULL;
>>> +     ...>
>>> + }
>>
>> rule1 matches like rule0, except the Error ** parameter match is
>> tightened from any C identifier to the C identifier errp, and the
>> function body match tightened from "either use
>> error_prepend/error_append_hint or which use local_err+error_propagate
>> to check errors" to just the latter.
>>
>> I figure tightening the Error ** parameter match has no effect, because
>> we already normalized the parameter name.
>>
>> So rule1 deletes variable local_err where rule0 applied.  Correct?
>>
>>> +
>>> +@@
>>> +// Handle pattern with goto, otherwise we'll finish up
>>> +// with labels at function end which will not compile.
>>> +identifier rule1.fn, rule1.local_err;
>>> +identifier OUT;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +-    goto OUT;
>>> ++    return;
>>> +     ...>
>>> +- OUT:
>>> +-    error_propagate(errp, local_err);
>>> + }
>>
>> This is one special case of error_propagate() deletion.  It additionally
>> gets rid of a goto we no longer want.  For the general case, see below.
>>
>> The rule applies only where rule1 just deleted the variable.  Thus, the
>> two rules work in tandem.  Makes sense.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>
>> This rule also works in tandem with rule1.
>>
>>> +expression list args; // to reindent error_propagate_prepend
>>
>> What is the comment trying to tell me?
>>
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    error_free(local_err);
>>> +-    local_err = NULL;
>>> ++    error_free_errp(errp);
>>
>> Reminder:
>>
>>      static inline void error_free_errp(Error **errp)
>>      {
>>          assert(errp && *errp);
>>          error_free(*errp);
>>          *errp = NULL;
>>      }
>>
>> Now let's examine the actual change.
>>
>> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
>> ensures it.
>>
>> The second half is new.  We now crash when we haven't set an error.  Why
>> is this safe?  Note that error_free(local_err) does nothing when
>> !local_err.
>>
>> The zapping of the variable pointing to the Error just freed is
>> unchanged.
>>
>>> +|
>>> +-    error_free(local_err);
>>> ++    error_free_errp(errp);
>>
>> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
>> Needed, or else the automatic error_propagate() due to
>> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
>>
>>> +|
>>> +-    error_report_err(local_err);
>>> ++    error_report_errp(errp);

error_reportf_err() is just like error_report_err(), except it
additionally modifies the error message.  Does it need a similar
transformation?

>> The only difference to the previous case is that we also report the
>> error.
>>
>> The previous case has a buddy that additionally matches *errp = NULL.
>> Why not this one?
>>
>>> +|
>>> +-    warn_report_err(local_err);
>>> ++    warn_report_errp(errp);

Likewise.

>> Likewise.
>>
>> What about error_reportf_err(), warn_reportf_err()?
>>
>> Up to here, this rule transforms the various forms of error_free().
>> Next: error_propagate().
>>
>>> +|
>>> +-    error_propagate_prepend(errp, local_err, args);
>>> ++    error_prepend(errp, args);
>>> +|
>>> +-    error_propagate(errp, local_err);
>>
>> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
>> redundant.
>>
>> This is the general case of error_propagate() deletion.
>>
>> I'd put the plain error_propagate() first, variations second, like you
>> do with error_free().
>>
>> If neither of these two patterns match on a path from
>> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
>> where it wasn't before.  Does nothing when the local error is null
>> there.  Bug fix when it isn't: it's at least a memory leak, and quite
>> possibly worse.
>>
>> Identifying these bug fixes would be nice, but I don't have practical
>> ideas on how to do that.
>>
>> Can we explain this in the commit message?
>>
>>> +)
>>> +     ...>
>>> + }
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    &local_err
>>> ++    errp
>>> +|
>>> +-    local_err
>>> ++    *errp
>>> +)
>>> +     ...>
>>> + }
>>
>> Also in tandem with rule1, fixes up uses of local_err.  Good.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +- *errp != NULL
>>> ++ *errp
>>> +     ...>
>>> + }
>>
>> Still in tandem with rule1, normalizes style.  Good.
>>