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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <address@hidden> writes:

> 09.03.2020 12:56, Markus Armbruster wrote:
>> Suggest
>>
>>      scripts: Coccinelle script to use auto-propagated errp
>>
>> or
>>
>>      scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
>>
>> Vladimir Sementsov-Ogievskiy <address@hidden> 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]
>>
>> Suggest FILES... instead of a specific set of files.
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>
>>> Cc: Eric Blake <address@hidden>
>>> Cc: Kevin Wolf <address@hidden>
>>> Cc: Max Reitz <address@hidden>
>>> Cc: Greg Kurz <address@hidden>
>>> Cc: Christian Schoenebeck <address@hidden>
>>> Cc: Stefano Stabellini <address@hidden>
>>> Cc: Anthony Perard <address@hidden>
>>> Cc: Paul Durrant <address@hidden>
>>> Cc: Stefan Hajnoczi <address@hidden>
>>> Cc: "Philippe Mathieu-Daudé" <address@hidden>
>>> Cc: Laszlo Ersek <address@hidden>
>>> Cc: Gerd Hoffmann <address@hidden>
>>> Cc: Stefan Berger <address@hidden>
>>> Cc: Markus Armbruster <address@hidden>
>>> Cc: Michael Roth <address@hidden>
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>>
>>>   include/qapi/error.h                          |   3 +
>>>   scripts/coccinelle/auto-propagated-errp.cocci | 231 ++++++++++++++++++
>>>   2 files changed, 234 insertions(+)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index bb9bcf02fb..fbfc6f1c0b 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -211,6 +211,9 @@
>>>    *         }
>>>    *         ...
>>>    *     }
>>> + *
>>> + * For mass conversion use script
>>
>> mass-conversion (we're not converting mass, we're converting en masse)
>>
>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>>    */
>>>     #ifndef ERROR_H
>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
>>> b/scripts/coccinelle/auto-propagated-errp.cocci
>>> new file mode 100644
>>> index 0000000000..bff274bd6d
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>
>> Preface to my review of this script: may aim isn't to make it
>> bullet-proof.  I want to (1) make it good enough (explained in a
>> jiffie), and (2) automatically identify the spots where it still isn't
>> obviously safe for manual review.
>>
>> The latter may involve additional scripting.  That's okay.
>>
>> The script is good enough when the number of possibly unsafe spots is
>> low enough for careful manual review.
>>
>> When I ask for improvements that, in your opinion, go beyond "good
>> enough", please push back.  I'm sure we can work it out together.
>>
>>> @@ -0,0 +1,231 @@
>>> +// 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 \
>>> +//  --max-width 80 blockdev-nbd.c qemu-nbd.c \
>>
>> You have --max-width 80 here, but not in the commit message.  Default
>> seems to be 78.  Any particular reason to change it to 80?
>
> Hmm. As I remember, without this parameter, reindenting doesn't work 
> correctly.
> So, I'm OK with "--max-width 78", but I doubt that it will work without a 
> parameter.
> Still, may be I'm wrong, we can check it.

If you can point to an example where --max-width helps, keep it, and
update the commit message to match.  Else, drop it.

>>
>>> +//  {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>> +
>>> +// Switch unusual (Error **) parameter names to errp
>>
>> Let's drop the parenthesis around Error **
>>
>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
>>
>> Perhaps ERRP_AUTO_PROPAGATE() should be ERRP_AUTO_PROPAGATE(errp) to
>> make the fact we're messing with @errp more obvious.  Too late; I
>> shouldn't rock the boat that much now.
>>
>>> +//
>>> +// Disable optional_qualifier to skip functions with "Error *const *errp"
>>> +// parameter.
>>> +//
>>> +// Skip functions with "assert(_errp && *_errp)" statement, as they have
>>> +// non generic semantics and may have unusual Error ** argument name for 
>>> purpose
>>
>> non-generic
>>
>> for a purpose
>>
>> Wrap comment lines around column 70, please.  It's easier to read.
>>
>> Maybe
>>
>>     // Skip functions with "assert(_errp && *_errp)" statement, because that
>>     // signals unusual semantics, and the parameter name may well serve a
>>     // purpose.
>
> Sounds good.
>
>>
>>> +// (like nbd_iter_channel_error()).
>>> +//
>>> +// Skip util/error.c to not touch, for example, error_propagate and
>>> +// error_propagate_prepend().
>>
>> error_propagate()
>>
>> I much appreciate your meticulous explanation of what you skip and why.
>>
>>> +@ depends on !(file in "util/error.c") disable optional_qualifier@
>>> +identifier fn;
>>> +identifier _errp != errp;
>>> +@@
>>> +
>>> + fn(...,
>>> +-   Error **_errp
>>> ++   Error **errp
>>> +    ,...)
>>> + {
>>> +(
>>> +     ... when != assert(_errp && *_errp)
>>> +&
>>> +     <...
>>> +-    _errp
>>> ++    errp
>>> +     ...>
>>> +)
>>> + }
>>
>> This rule is required to make the actual transformations (below) work
>> even for parameters with names other than @errp.  I believe it's not
>> used in this series.  In fact, I can't see a use for it in the entire
>> tree right now.  Okay anyway.
>>
>>> +
>>> +// Add invocation of ERRP_AUTO_PROPAGATE to errp-functions where necessary
>>> +//
>>> +// Note, that without "when any" final "..." may not want to mach something
>>
>> s/final "..." may not mach/the final "..." does not match/
>>
>>> +// matched by previous pattern, i.e. the rule will not match double
>>> +// error_prepend in control flow like in vfio_set_irq_signaling().
>>
>> Can't say I fully understand Coccinelle there.  I figure you came to
>> this knowledge the hard way.
>
> It's follows from smpl grammar document:
>
> "Implicitly, “...” matches the shortest path between something that matches 
> the pattern before the dots (or the beginning of the function, if there is 
> nothing before the dots) and something that matches the pattern after the 
> dots (or the end of the function, if there is nothing after the dots)."
> ...
> "_when any_ removes the aforementioned constraint that “...” matches the 
> shortest path"

Let me think that through.

The pattern with the cases other than error_prepend() omitted:

     fn(..., Error **errp, ...)
     {
    +   ERRP_AUTO_PROPAGATE();
        ...  when != ERRP_AUTO_PROPAGATE();
        error_prepend(errp, ...);
        ... when any
     }

Tail of vfio_set_irq_signaling():

        name = index_to_str(vbasedev, index);
        if (name) {
            error_prepend(errp, "%s-%d: ", name, subindex);
        } else {
            error_prepend(errp, "index %d-%d: ", index, subindex);
        }
        error_prepend(errp,
                      "Failed to %s %s eventfd signaling for interrupt ",
                      fd < 0 ? "tear down" : "set up", action_to_str(action));
        return ret;
    }

The pattern's first ... matches a "shortest" path to an error_prepend(),
where "shortest" means "does not cross an error_prepend().  Its when
clause makes us ignore functions that already use ERRP_AUTO_PROPAGATE().

There are two such "shortest" paths, one to the first error_prepend() in
vfio_set_irq_signaling(), and one to the second.  Neither path to the
third one is not "shortest": they both cross one of the other two
error_prepend().

The pattern' s second ... matches a path from a matched error_prepend()
to the end of the function.  There are two paths.  Both cross the third
error_prepend().  You need "when any" to make the pattern match anyway.

Alright, I think I got it.  But now I'm paranoid about ... elsewhere.
For instance, here's rule1 with error_propagate_prepend() omitted:

    // Match scenarios with propagation of local error to errp.
    @rule1 disable optional_qualifier exists@
    identifier fn, local_err;
    symbol errp;
    @@

     fn(..., Error **errp, ...)
     {
         ...
         Error *local_err = NULL;
         ...
         error_propagate(errp, local_err);
         ...
     }

The second and third ... won't match anything containing
error_propagate().  What if a function has multiple error_propagate() on
all paths?  Like this one:

    extern foo(int, Error **);
    extern bar(int, Error **);

    void frob(Error **errp)
    {
        Error *local_err = NULL;
        int arg;

        foo(arg, errp);
        bar(arg, &local_err);
        error_propagate(errp, local_err);
        bar(arg + 1, &local_err);
        error_propagate(errp, local_err);
    }

This is actually a variation of error.h's "Receive and accumulate
multiple errors (first one wins)" code snippet.

The Coccinelle script transforms it like this:

     void frob(Error **errp)
     {
    +    ERRP_AUTO_PROPAGATE();
         Error *local_err = NULL;
         int arg;

The rule that adds ERRP_AUTO_PROPAGATE() matches (it has ... when any),
but rule1 does not, and we therefore don't convert any of the
error_propagate().

The result isn't wrong, just useless.

Is this the worst case?

Possible improvement to the ERRP_AUTO_PROPAGATE() rule: don't use
"... when any" in the error_propagate() case, only in the other cases.
Would that help?

I think this is the only other rule with "..." matching control flow.

>>
>>> +//
>>> +// Note, "exists" says that we want apply rule even if it matches not on
>>> +// all possible control flows (otherwise, it will not match standard 
>>> pattern
>>> +// when error_propagate() call is in if branch).
>>
>> Learned something new.  Example: kvm_set_kvm_shadow_mem().
>>
>> Spelling it "exists disable optional_qualifier" would avoid giving
>> readers the idea we're disabling "exists", but Coccinelle doesn't let
>> us.  Oh well.
>>
>>> +@ disable optional_qualifier exists@
>>> +identifier fn, local_err, errp;
>>
>> I believe this causes
>>
>>      warning: line 98: errp, previously declared as a metavariable, is used 
>> as an identifier
>>      warning: line 104: errp, previously declared as a metavariable, is used 
>> as an identifier
>>      warning: line 106: errp, previously declared as a metavariable, is used 
>> as an identifier
>>      warning: line 131: errp, previously declared as a metavariable, is used 
>> as an identifier
>>      warning: line 192: errp, previously declared as a metavariable, is used 
>> as an identifier
>>      warning: line 195: errp, previously declared as a metavariable, is used 
>> as an identifier
>>      warning: line 228: errp, previously declared as a metavariable, is used 
>> as an identifier
>>
>> Making @errp symbol instead of identifier should fix this.
>
> Hmm, I didn't see these warnings.. But yes, it should be symbol.
>
>>
>>> +@@
>>> +
>>> + fn(..., Error **errp, ...)
>>> + {
>>> ++   ERRP_AUTO_PROPAGATE();
>>> +    ...  when != ERRP_AUTO_PROPAGATE();
>>> +(
>>> +    error_append_hint(errp, ...);
>>> +|
>>> +    error_prepend(errp, ...);
>>> +|
>>> +    error_vprepend(errp, ...);
>>> +|
>>> +    Error *local_err = NULL;
>>> +    ...
>>> +(
>>> +    error_propagate_prepend(errp, local_err, ...);
>>> +|
>>> +    error_propagate(errp, local_err);
>>> +)
>>> +)
>>> +    ... when any
>>> + }
>>> +
>>> +
>>> +// Match scenarios with propagation of local error to errp.
>>> +@rule1 disable optional_qualifier exists@
>>> +identifier fn, local_err;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(..., Error **errp, ...)
>>> + {
>>> +     ...
>>> +     Error *local_err = NULL;
>>> +     ...
>>> +(
>>> +    error_propagate_prepend(errp, local_err, ...);
>>> +|
>>> +    error_propagate(errp, local_err);
>>> +)
>>
>> Indentation off by one.
>>
>>> +     ...
>>> + }
>>> +
>>> +// Convert special case with goto in separate.
>>
>> s/in separate/separately/
>>
>>> +// We can probably merge this into the following hunk with help of ( | )
>>> +// operator, but it significantly reduce performance on block.c parsing 
>>> (or it
>>
>> s/reduce/reduces/
>>
>>> +// hangs, I don't know)
>>
>> Sounds like you tried to merge this into the following hunk, but then
>> spatch took so long on block.c that you killed it.  Correct?
>
> Yes.

I'd say something like "I tried merging this into the following rule the
obvious way, but it made Coccinelle hang on block.c."

>>
>>> +//
>>> +// Note interesting thing: if we don't do it here, and try to fixup "out: 
>>> }"
>>> +// things later after all transformations (the rule will be the same, just
>>> +// without error_propagate() call), coccinelle fails to match this "out: 
>>> }".
>>
>> Weird, but not worth further investigation.
>
> It partially match to the idea which I saw somewhere in coccinelle 
> documentation,
> that coccinelle converts correct C code to correct C code. "out: }" is an 
> example
> of incorrect, impossible code flow, and coccinelle can't work with it... But 
> it's
> just a thought.
>
>>
>>> +@@
>>> +identifier rule1.fn, rule1.local_err, out;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +-    goto out;
>>> ++    return;
>>> +     ...>
>>> +- out:
>>> +-    error_propagate(errp, local_err);
>>
>> You neglect to match error_propagate_prepend().  Okay, because (1) that
>> pattern doesn't occur in the tree right now, and (2) if it gets added,
>> gcc will complain.
>
> No, because it should not removed. error_propagate_prepend should be converted
> to prepend, not removed. So, corresponding gotos should not be removed as 
> well.

You're right.

>>
>>> + }
>>> +
>>> +// Convert most of local_err related staff.
>>
>> s/staff/stuff/
>>
>>> +//
>>> +// Note, that we update everything related to matched by rule1 function 
>>> name
>>> +// and local_err name. We may match something not related to the pattern
>>> +// matched by rule1. For example, local_err may be defined with the same 
>>> name
>>> +// in different blocks inside one function, and in one block follow the
>>> +// propagation pattern and in other block doesn't. Or we may have several
>>> +// functions with the same name (for different configurations).
>>
>> Context: rule1 matches functions that have all three of
>>
>> * an Error **errp parameter
>>
>> * an Error *local_err = NULL variable declaration
>>
>> * an error_propagate(errp, local_err) or error_propagate_prepend(errp,
>>    local_err, ...) expression, where @errp is the parameter and
>>    @local_err is the variable.
>>
>> If I understand you correctly, you're pointing out two potential issues:
>>
>> 1. This rule can match functions rule1 does not match if there is
>> another function with the same name that rule1 does match.
>>
>> 2. This rule matches in the entire function matched by rule1, even when
>> parts of that function use a different @errp or @local_err.
>>
>> I figure these apply to all rules with identifier rule1.fn, not just
>> this one.  Correct?
>
> Yes.

Thanks!

>>
>> Regarding 1.  There must be a better way to chain rules together, but I
>> don't know it.
>>  Can we make Coccinelle at least warn us when it converts
>> multiple functions with the same name?  What about this:
>>
>>     @initialize:python@
>>     @@
>>     fnprev = {}
>>
>>     def pr(fn, p):
>>         print("### %s:%s: %s()" % (p[0].file, p[0].line, fn))
>>
>>     @r@
>>     identifier rule1.fn;
>>     position p;
>>     @@
>>      fn(...)@p
>>      {
>>          ...
>>      }
>>     @script:python@
>>         fn << rule1.fn;
>>         p << r.p;
>>     @@
>>     if fn not in fnprev:
>>         fnprev[fn] = p
>>     else:
>>         if fnprev[fn]:
>
> hmm, the condition can't be false
>
>>             pr(fn, fnprev[fn])
>>             fnprev[fn] = None
>>         pr(fn, p)
>
> and we'll miss next duplication..

The idea is

    first instance of fn:
        fn not in fnprev
        fnprev[fn] = position of instance
        don't print
    second instance:
        fnprev[fn] is the position of the first instance
        print first two instances
    subsequent instances: fnprev[fn] is None
        print this instance

I might have screwed up the coding, of course :)

> But I like the idea.
>
>>
>> For each function @fn matched by rule1, fncnt[fn] is an upper limit of
>> the number of functions with the same name we touch.  If it's more than
>> one, we print.
>>
>> Reports about a dozen function names for the whole tree in my testing.
>> Inspecting the changes to them manually is feasible.  None of them are
>> in files touched by this series.
>>
>> The line printed for the first match is pretty useless for me: it points
>> to a Coccinelle temporary file *shrug*.
>>
>> Regarding 2.  Shadowing @errp or @local_err would be in bad taste, and I
>> sure hope we don't do that.  Multiple @local_err variables... hmm.
>> Perhaps we could again concoct some script rules to lead us to spots to
>> check manually.  See below for my attempt.
>>
>> What's the worst that could happen if we blindly converted such code?
>> The answer to that question tells us how hard to work on finding and
>> checking these guys.
>>
>>> +//
>>> +// Note also that errp-cleaning functions
>>> +//   error_free_errp
>>> +//   error_report_errp
>>> +//   error_reportf_errp
>>> +//   warn_report_errp
>>> +//   warn_reportf_errp
>>> +// are not yet implemented. They must call corresponding Error* - freeing
>>> +// function and then set *errp to NULL, to avoid further propagation to
>>> +// original errp (consider ERRP_AUTO_PROPAGATE in use).
>>> +// For example, error_free_errp may look like this:
>>> +//
>>> +//    void error_free_errp(Error **errp)
>>> +//    {
>>> +//        error_free(*errp);
>>> +//        *errp = NULL;
>>> +//    }
>>> +@ exists@
>>> +identifier rule1.fn, rule1.local_err;
>>> +expression list args;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>
>> Each of the following patterns applies anywhere in the function.
>>
>> First pattern: delete @local_err
>>
>>> +-    Error *local_err = NULL;
>>
>> Common case: occurs just once, not nested.  Anything else is suspicious.
>>
>> Both can be detected in the resulting patches with a bit of AWK
>> wizardry:
>>
>>      $ git-diff -U0 master..review-error-v8 | awk '/^@@ / { ctx = $5; for (i 
>> = 6; i <= NF; i++) ctx = ctx " " $i; if (ctx != octx) { octx = ctx; n = 0 } 
>> } /^- *Error *\* *[A-Za-z0-9_]+ *= *NULL;/ { if (index($0, "E") > 6) print 
>> "nested\n    " ctx; if (n) print "more than one\n    " ctx; n++ }'
>>      nested
>>          static void xen_block_drive_destroy(XenBlockDrive *drive, Error 
>> **errp)
>>      nested
>>          static void xen_block_device_destroy(XenBackendInstance *backend,
>>      nested
>>          static void xen_block_device_destroy(XenBackendInstance *backend,
>>      more than one
>>          static void xen_block_device_destroy(XenBackendInstance *backend,
>>
>> Oh.
>>
>> xen_block_drive_destroy() nests its Error *local_err in a conditional.
>>
>> xen_block_device_destroy() has multiple Error *local_err.
>>
>> In both cases, manual review is required to ensure the conversion is
>> okay.  I believe it is.
>>
>> Note that the AWK script relies on diff showing the function name in @@
>> lines, which doesn't always work due to our coding style.
>>
>> For the whole tree, I get some 30 spots.  Feasible.
>>
>>> +|
>>
>> Second pattern: clear @errp after freeing it
>>
>>> +
>>> +// Convert error clearing functions
>>
>> Suggest: Ensure @local_err is cleared on free
>>
>>> +(
>>> +-    error_free(local_err);
>>> ++    error_free_errp(errp);
>>> +|
>>> +-    error_report_err(local_err);
>>> ++    error_report_errp(errp);
>>> +|
>>> +-    error_reportf_err(local_err, args);
>>> ++    error_reportf_errp(errp, args);
>>> +|
>>> +-    warn_report_err(local_err);
>>> ++    warn_report_errp(errp);
>>> +|
>>> +-    warn_reportf_err(local_err, args);
>>> ++    warn_reportf_errp(errp, args);
>>> +)
>>
>> As you mention above, these guys don't exist, yet.  Builds anyway,
>> because this part of the rule is not used in this patch series.  You
>> don't want to omit it, because then the script becomes unsafe to use.
>>
>> We could also open-code:
>>
>>     // Convert error clearing functions
>>     (
>>     -    error_free(local_err);
>>     +    error_free(*errp);
>>     +    *errp = NULL;
>>     |
>>     ... and so forth ...
>>     )
>>
>> Matter of taste.  Whatever is easier to explain in the comments.  Since
>> you already wrote one...
>
> I just feel that using helper functions is safer way..
>
>>
>> We talked about extending this series slightly so these guys are used.
>> I may still look into that.
>>
>>> +?-    local_err = NULL;
>>> +
>>
>> The new helpers clear @local_err.  Assignment now redundant, delete.
>> Okay.
>>
>>> +|
>>
>> Third and fourth pattern: delete error_propagate()
>>
>>> +-    error_propagate_prepend(errp, local_err, args);
>>> ++    error_prepend(errp, args);
>>> +|
>>> +-    error_propagate(errp, local_err);
>>> +|
>>
>> Fifth pattern: use @errp directly
>>
>>> +-    &local_err
>>> ++    errp
>>> +)
>>> +     ...>
>>> + }
>>> +
>>> +// Convert remaining local_err usage. It should be different kinds of error
>>> +// checking in if operators. We can't merge this into previous hunk, as 
>>> this
>>
>> In if conditionals, I suppose.  It's the case for this patch.  If I
>> apply the script to the whole tree, the rule gets also applied in other
>> contexts.  The sentence might mislead as much as it helps.  Keep it or
>> delete it?
>
> Maybe, just be more honest: "It should be ..., but it may be any other 
> pattern, be careful"

"Need to be careful" means "needs careful manual review", which I
believe is not feasible; see "Preface to my review of this script"
above.

But do we really need to be careful here?

This rule should apply only where we added ERRP_AUTO_PROPAGATE().

Except when rule chaining via function name fails us, but we plan to
detect that and review manually, so let's ignore this issue here.

Thanks to ERRP_AUTO_PROPAGATE(), @errp is not null.  Enabling
replacement of @local_err by @errp is its whole point.

What exactly do we need to be careful about?

>
>>
>>> +// conflicts with other substitutions in it (at least with "- local_err = 
>>> NULL").
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +-    local_err
>>> ++    *errp
>>> +     ...>
>>> + }
>>> +
>>> +// Always use the same patter for checking error
>>
>> s/patter/pattern/
>>
>>> +@@
>>> +identifier rule1.fn;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +-    *errp != NULL
>>> ++    *errp
>>> +     ...>
>>> + }
>>