[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
- To: Markus Armbruster <armbru@xxxxxxxxxx>
- From: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
- Date: Tue, 25 Feb 2020 18:22:50 +0300
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=virtuozzo.com; dmarc=pass action=none header.from=virtuozzo.com; dkim=pass header.d=virtuozzo.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=GA6OdMo+HRYFcpRz4GY+QG/za/1o8rXZz0UH8sfdhcQ=; b=gBQwTo2Ag5LmBe10U+Bk6VtWeJQbodOWavKGvZXF2DE3ZWjw6If8aMLNMaEZ97joR4vt17O8FvMqAmP5g2k03O6JlILeSEFk8nV8l2iJOt7c5KhlvpNqkh4AhxMGD18Bo8C9iDk10uiivPVG8lmE5km8+yW+zhE56EAQL3KAjPwTpwO22kMsgv0yBU5WTc2JDEdFyA93RCsMSjSWaKHKhgPNA10bUaDD7z62pGqy+wkOzQ0aOic2lMIloDg40UfKMpIV9EtkcYMJHWCY1VCZ5QzQj/dBjJSuztlvTsZft1Rif4lyyZDO8eylbRt/254/PgZMHVJw6YiWcjpqoKlFQQ==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bltRUm8+7vp7fI8uBGMZKdWpcv0dkBMsAlL6ckFkImKiqLe9HIq2v5d4WVgmpqJjWV0DfFHlA/UCyHNj2Mr5wLNN57a8FFH0k2LY9Sk8oBhUlzX1xNNx59Ulay84sp+K0yRdJv7So62AbP2ujb6CHg2wg+cFJ6SDGD+ZDpZyrf6f3/tkaPP9t5EGDA/x1XjxO+g+MryRODaVeUhw1fH5U23sBuYQ7nPIT1dqyIqI63gMNTcX/yyaT8p7HW03ECb5Pk+6zxt1/cyX5sjhh6gVjAITuic9X9AUExL5huiIZdS6tVXbTYWp0qZ6rwDy0H9B4+4yN6CNoDdjgD+QZf8ZQw==
- Authentication-results: spf=none (sender IP is ) smtp.mailfrom=vsementsov@xxxxxxxxxxxxx;
- Cc: Kevin Wolf <kwolf@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, qemu-block@xxxxxxxxxx, Paul Durrant <paul@xxxxxxx>, Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>, Michael Roth <mdroth@xxxxxxxxxxxxxxxxxx>, qemu-devel@xxxxxxxxxx, Greg Kurz <groug@xxxxxxxx>, Gerd Hoffmann <kraxel@xxxxxxxxxx>, Stefan Hajnoczi <stefanha@xxxxxxxxxx>, Anthony Perard <anthony.perard@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Max Reitz <mreitz@xxxxxxxxxx>, Laszlo Ersek <lersek@xxxxxxxxxx>, Stefan Berger <stefanb@xxxxxxxxxxxxx>
- Delivery-date: Tue, 25 Feb 2020 15:23:04 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
25.02.2020 15:52, Markus Armbruster wrote:
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?
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.
--
Best regards,
Vladimir
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
