[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v8 02/10] scripts: add coccinelle script to use auto propagated errp
- To: Markus Armbruster <armbru@xxxxxxxxxx>
- From: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
- Date: Wed, 11 Mar 2020 11:32:22 +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=ZhdzVze29VFot6BAq2eXp3PIhkhirf9Q0k1Lbtofrts=; b=SOiqnUTjUi0PBhhNrc6wjVrdHwmp0Sa2NN9nLf3MeUzvs02NhZQaSgCefp+tykMY8GKajb22eKMID/HBwmAIgDOS9Vd9zECuJ3A4eqy41PLnqdgVMX0cHQVmbqD6xYfRuvYLcAn38Kbr+AOkwPPcAFSE4L6YIO/aED+nZWXWZnTfBFhUtYJ6ZQJyhZaZ2ThkMdHty2W1aJHSb5a+r1KHV+Ym6UQNhVXzg3AxZE7pxVhZX+fJ2ie1faLLq1dWbPKTATY3T0v1ZJvFvpxaGTlK5g0UeTKBQVZYJJIQzvkNoi1fuwJtFVX9PQ9OkCsw8KSiiVb2W2tezCCecf+t+ZiIUw==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jEk7UxwH2tEfsTnZ05W+ljw/WWhxRT91iTTwZEMuNugCnDcCPAZCb8OwblrBTIUquro9R50lVbUn89qSffRceYXMdVGs9Iz1bnLBEg0LzxA+JUBGhfyWyxYT1jae0I59a+iY4FFSjhJ3ST5EU82fIJW1q9b9foZzlSIxP0p42PbOx851qHWBqDyck9dYs3AGUdI52rNAgJas5kKQkUWZ0s4nZh1w6KxgHUbtEXLTTtI+izMjOiNSQuLY84FzLux+pRbumruHqx82ZW7VHctGQ6nrQizo1kDzB9uykVoMDZ1R+jpxQYjyY/tzEbgNcxMI6lDTG2fTWZ6A8Zb3gDWXHQ==
- 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>, Christian Schoenebeck <qemu_oss@xxxxxxxxxxxxx>, 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: Wed, 11 Mar 2020 08:32:37 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
11.03.2020 9:55, Vladimir Sementsov-Ogievskiy wrote:
10.03.2020 18:47, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
09.03.2020 12:56, Markus Armbruster wrote:
Suggest
scripts: Coccinelle script to use auto-propagated errp
or
scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
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 <vsementsov@xxxxxxxxxxxxx>
---
Cc: Eric Blake <eblake@xxxxxxxxxx>
Cc: Kevin Wolf <kwolf@xxxxxxxxxx>
Cc: Max Reitz <mreitz@xxxxxxxxxx>
Cc: Greg Kurz <groug@xxxxxxxx>
Cc: Christian Schoenebeck <qemu_oss@xxxxxxxxxxxxx>
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: qemu-devel@xxxxxxxxxx
Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
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?
I thought about this, but decided that double error propagation is a strange
pattern, and may be better not match it...
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.
ah yes, we can propagate to already filled errp, which just clean local_err.
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 not, as it will anyway match functions with error_prepend (and any
number of following error_propagate calls)...
But it's correct to add ERRP_AUTO_PROPAGATE to such functions.. So, may be you
are right. Let's do it, seems better.
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?
Hmm.. About some unpredicted patterns. OK, then "For example, different kinds of
.."
+// 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
+ ...>
+ }
--
Best regards,
Vladimir
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
