|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp
Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
> 23.02.2020 11:55, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>> ---
>>>
>>> CC: Eric Blake <eblake@xxxxxxxxxx>
>>> CC: Kevin Wolf <kwolf@xxxxxxxxxx>
>>> CC: Max Reitz <mreitz@xxxxxxxxxx>
>>> CC: Greg Kurz <groug@xxxxxxxx>
>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
>>> CC: Paul Durrant <paul@xxxxxxx>
>>> CC: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
>>> CC: "Philippe Mathieu-Daudé" <philmd@xxxxxxxxxx>
>>> CC: Laszlo Ersek <lersek@xxxxxxxxxx>
>>> CC: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>>> CC: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>>> CC: Markus Armbruster <armbru@xxxxxxxxxx>
>>> CC: Michael Roth <mdroth@xxxxxxxxxxxxxxxxxx>
>>> CC: qemu-block@xxxxxxxxxx
>>> CC: xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>
>>> 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.
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |