|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes: > 13.03.2020 18:42, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes: >> >>> 12.03.2020 19:36, Markus Armbruster wrote: >>>> I may have a second look tomorrow with fresher eyes, but let's get this >>>> out now as is. >>>> >>>> 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 \ >>>>> --max-width 80 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-devel@xxxxxxxxxx >>>>> Cc: qemu-block@xxxxxxxxxx >>>>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx >>>>> >>>>> scripts/coccinelle/auto-propagated-errp.cocci | 327 ++++++++++++++++++ >>>>> include/qapi/error.h | 3 + >>>>> MAINTAINERS | 1 + >>>>> 3 files changed, 331 insertions(+) >>>>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci >>>>> >>>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci >>>>> b/scripts/coccinelle/auto-propagated-errp.cocci >>>>> new file mode 100644 >>>>> index 0000000000..7dac2dcfa4 >>>>> --- /dev/null >>>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci >>>>> @@ -0,0 +1,327 @@ >>>>> +// 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 FILES... >>>>> +// >>>>> +// Note: --max-width 80 is needed because coccinelle default is less >>>>> +// than 80, and without this parameter coccinelle may reindent some >>>>> +// lines which fit into 80 characters but not to coccinelle default, >>>>> +// which in turn produces extra patch hunks for no reason. >>>> >>>> This is about unwanted reformatting of parameter lists due to the ___ >>>> chaining hack. --max-width 80 makes that less likely, but not >>>> impossible. >>>> >>>> We can search for unwanted reformatting of parameter lists. I think >>>> grepping diffs for '^\+.*Error \*\*' should do the trick. For the whole >>>> tree, I get one false positive (not a parameter list), and one hit: >>>> >>>> @@ -388,8 +388,10 @@ static void object_post_init_with_type(O >>>> } >>>> } >>>> >>>> -void object_apply_global_props(Object *obj, const GPtrArray *props, >>>> Error **errp) >>>> +void object_apply_global_props(Object *obj, const GPtrArray *props, >>>> + Error **errp) >>>> { >>>> + ERRP_AUTO_PROPAGATE(); >>>> int i; >>>> >>>> if (!props) { >>>> >>>> Reformatting, but not unwanted. >>> >>> Yes, I saw it. This line is 81 character length, so it's OK to fix it in >>> one hunk with >>> ERRP_AUTO_PROPAGATE addition even for non-automatic patch. >> >> Agree. >> >>>> >>>> The --max-width 80 hack is good enough for me. >>>> >>>> It does result in slightly long transformed lines, e.g. this one in >>>> replication.c: >>>> >>>> @@ -113,7 +113,7 @@ static int replication_open(BlockDriverS >>>> s->mode = REPLICATION_MODE_PRIMARY; >>>> top_id = qemu_opt_get(opts, REPLICATION_TOP_ID); >>>> if (top_id) { >>>> - error_setg(&local_err, "The primary side does not >>>> support option top-id"); >>>> + error_setg(errp, "The primary side does not support >>>> option top-id"); >>>> goto fail; >>>> } >>>> } else if (!strcmp(mode, "secondary")) { >>>> >>>> v8 did break this line (that's how I found it). However, v9 still >>>> shortens the line, just not below the target. All your + lines look >>>> quite unlikely to lengthen lines. Let's not worry about this. >>>> >>>>> +// Switch unusual Error ** parameter names to errp >>>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE). >>>>> +// >>>>> +// Disable optional_qualifier to skip functions with >>>>> +// "Error *const *errp" parameter. >>>>> +// >>>>> +// Skip functions with "assert(_errp && *_errp)" statement, because >>>>> +// that signals unusual semantics, and the parameter name may well >>>>> +// serve a purpose. (like nbd_iter_channel_error()). >>>>> +// >>>>> +// Skip util/error.c to not touch, for example, error_propagate() and >>>>> +// error_propagate_prepend(). >>>>> +@ 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 >>>>> + ...> >>>>> +) >>>>> + } >>>>> + >>>>> +// Add invocation of ERRP_AUTO_PROPAGATE to errp-functions where >>>>> +// necessary >>>>> +// >>>>> +// Note, that without "when any" the final "..." does not mach >>>>> +// something matched by previous pattern, i.e. the rule will not match >>>>> +// double error_prepend in control flow like in >>>>> +// vfio_set_irq_signaling(). >>>>> +// >>>>> +// 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). >>>>> +@ disable optional_qualifier exists@ >>>>> +identifier fn, local_err; >>>>> +symbol errp; >>>>> +@@ >>>>> + >>>>> + fn(..., Error **errp, ...) >>>>> + { >>>>> ++ ERRP_AUTO_PROPAGATE(); >>>>> + ... when != ERRP_AUTO_PROPAGATE(); >>>>> +( >>>>> +( >>>>> + error_append_hint(errp, ...); >>>>> +| >>>>> + error_prepend(errp, ...); >>>>> +| >>>>> + error_vprepend(errp, ...); >>>>> +) >>>>> + ... when any >>>>> +| >>>>> + Error *local_err = NULL; >>>>> + ... >>>>> +( >>>>> + error_propagate_prepend(errp, local_err, ...); >>>>> +| >>>>> + error_propagate(errp, local_err); >>>>> +) >>>>> + ... >>>>> +) >>>>> + } >>>>> + >>>>> + >>>>> +// Match functions with propagation of local error to errp. >>>>> +// We want to refer these functions in several following rules, but I >>>>> +// don't know a proper way to inherit a function, not just its name >>>>> +// (to not match another functions with same name in following rules). >>>>> +// Not-proper way is as follows: rename errp parameter in functions >>>>> +// header and match it in following rules. Rename it back after all >>>>> +// transformations. >>>>> +// >>>>> +// The simplest case of propagation scheme is single definition of >>>>> +// local_err with at most one error_propagate_prepend or >>>>> +// error_propagate on each control-flow. Still, we want to match more >>>>> +// complex schemes too. We'll warn them with help of further rules. >>>> >>>> I think what we actually want is to examine instances of this pattern to >>>> figure out whether and how we want to transform them. Perhaps: >>>> >>>> // The common case is a single definition of local_err with at most >>>> one >>>> // error_propagate_prepend() or error_propagate() on each >>>> control-flow >>>> // path. Instances of this case we convert with this script. >>>> Functions >>> >>> For me, sounds a bit like "other things we don't convert". >>> Actually we convert other things too. >> >> What other patterns of error propagation do we convert? > > Something like in xen_block_device_destroy, why not? Otherwise, it's better > to avoid > matching things like xen_block_device_destroy, not just warn them. > But I'd prefer to proceed now as is to fit into 5.0.. Too much time already > spent on this. So, I'm OK with your wording too. Let's scratch "Instances of this case we convert with this script." >>>> // with multiple definitions or propagates we want to examine >>>> // manually. Later rules emit warnings to guide us to them. >>>> >>>>> +@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); >>>>> +) >>>>> + ... >>>>> + } >>>>> + >>>>> + >>>>> +// Warn several Error * definitions. >>>>> +@check1 disable optional_qualifier exists@ >>>>> +identifier fn = rule1.fn, local_err, local_err2; >>>> >>>> Elsewhere, you use just rule.fn instead of fn = rule1.fn. Any >>>> particular reason for the difference? >>> >>> I didn't find other way to ref check1.fn in next python rule. It just don't >>> work if I write here just rule1.fn. >>> >>>> >>>> With the ___ chaining hack, I doubt we still need "= rule1.fn" or >>>> "rule1.fn". If I replace "fn = rule1.fn" and "rule.fn" by just "fn" >>>> everywhere, then apply the script to the complete tree, I get the same >>>> result. >>> >>> I think, it's more efficient to reuse names from previous rules. I think it >>> should >>> work faster (more information, less extra matching). >> >> Nope. With my hacked up script (patch appended) Coccinelle is actually >> *faster* for the .[ch] touched by this series: with your unmodified >> script, it takes a bit over 12s on my box, with mine around 7s. Output >> is identical. >> >> Never guess performance, always measure it :) > > Hmm, whole tree results would be better proof > >> >> Two notes on my script: >> >> * Unlike yours, it recognizes double-propagation in my test case. >> Discussed below. >> >> * Its "several definitions of" warning includes positions. That turned >> out to be useless, but I've been too lazy to take that out again. >> >>>> >>>>> +@@ >>>>> + >>>>> + fn(..., Error ** ____, ...) >>>>> + { >>>>> + ... >>>>> + Error *local_err = NULL; >>>>> + ... when any >>>>> + Error *local_err2 = NULL; >>>>> + ... when any >>>>> + } This flags functions that have more than one declaration along any control flow path. It doesn't flag this one: void gnat(bool b, Error **errp) { if (b) { Error *local_err = NULL; foo(arg, &local_err); error_propagate(errp, local_err); } else { Error *local_err = NULL; bar(arg, &local_err); error_propagate(errp, local_err); } } The Coccinelle script does the right thing for this one regardless. I'd prefer to have such functions flagged, too. But spending time on convincing Coccinelle to do it for me is not worthwhile; I can simply search the diff produced by Coccinelle for deletions of declarations that are not indented exactly four spaces. But if we keep this rule, we should adjust its comment // Warn several Error * definitions. because it sure suggests it also catches functions like the one I gave above. >>>>> + >>>>> +@ script:python @ >>>>> +fn << check1.fn; >>>>> +@@ >>>>> + >>>>> +print('Warning: function {} has several definitions of ' >>>>> + 'Error * local variable'.format(fn)) >>>>> + >>>>> +// Warn several propagations in control flow. >>>>> +@check2 disable optional_qualifier exists@ >>>>> +identifier fn = rule1.fn; >>>>> +symbol errp; >>>>> +position p1, p2; >>>>> +@@ >>>>> + >>>>> + fn(..., Error ** ____, ...) >>>>> + { >>>>> + ... >>>>> +( >>>>> + error_propagate_prepend(errp, ...);@p1 >>>>> +| >>>>> + error_propagate(errp, ...);@p1 >>>>> +) >>>>> + ... >>>>> +( >>>>> + error_propagate_prepend(errp, ...);@p2 >>>>> +| >>>>> + error_propagate(errp, ...);@p2 >>>>> +) >>>>> + ... when any >>>>> + } >>>>> + >>>> >>>> Hmm, we don't catch the example I used in review of v8: >>>> >>>> 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); >>>> } >>>> >>>> I believe this is because rule1 does not match here. >>> >>> Yes, rule1 wants at least one code flow with non-doubled propagation. >>> >>>> >>>> If I change the rule as follows, it catches the example: >>>> >>>> @@ -157,24 +157,23 @@ print('Warning: function {} has several >>>> definitions of ' >>>> >>>> // Warn several propagations in control flow. >>>> @check2 disable optional_qualifier exists@ >>>> -identifier fn = rule1.fn; >>>> -symbol errp; >>>> +identifier fn, _errp; >>>> position p1, p2; >>>> @@ >>>> >>>> - fn(..., Error ** ____, ...) >>>> + fn(..., Error **_errp, ...) >>>> { >>>> ... >>>> ( >>>> - error_propagate_prepend(errp, ...);@p1 >>>> + error_propagate_prepend(_errp, ...);@p1 >>>> | >>>> - error_propagate(errp, ...);@p1 >>>> + error_propagate(_errp, ...);@p1 >>>> ) >>>> ... >>>> ( >>>> - error_propagate_prepend(errp, ...);@p2 >>>> + error_propagate_prepend(_errp, ...);@p2 >>>> | >>>> - error_propagate(errp, ...);@p2 >>>> + error_propagate(_errp, ...);@p2 >>>> ) >>>> ... when any >>>> } >>>> >>>> To my mild surprise, it still doesn't find anything in our tree. >>>> >>>> Should we decouple the previous rule from rule1, too? I tested the >>>> following on the whole tree: >>> >>> I don't think so. Why to check what we are not going to convert? If we want >>> to check side things, it's better to do it in other coccinelle script.. >> >> Misunderstanding? The rules are still chained together via the ___ >> hack, just not via function name, because that's unreliable and >> redundant. > > Strange.. Then, how can it match something not matched by rule1? I think I got confused when I wrote the "Misunderstanding?" paragraph. Let me try again. First rule check2. The common case is a at most one propagation to @errp along any control flow path. We trust your Coccinelle script to convert that alright. Any other propagation to @errp I want to review. Whether the script attempts a conversion or not is unimportant, as long as it points me to the function to review. Rule rule1 matches functions that propagate to @errp once along at least one control flow path. Unchained from rule rule1, rule check2 flags any function that propagates to @errp multiple times along any control flow path. Chained to rule1, it flags only functions that also have a path with single propagation. In other words, the unchained rule flags *all* multi-propagations to @errp, while the chained rule flags only the ones the script attempts to convert. The former is much more useful to me. Now rule check1. It flags functions with multiple declarations along any control flow path. Again, chaining it to rule1 restricts it to the functions we attempt to convert. Makes it less useful to me. However, because my desire to review multiple declarations in function we don't attempt to convert is lower than my desire to review multiple propagations to @errp in such functions, chaining check1 is tolerable for me. But why chain check1 if we don't chain check2? > >> >>>> >>>> @@ -136,10 +136,10 @@ symbol errp; >>>> >>>> // Warn several Error * definitions. >>>> @check1 disable optional_qualifier exists@ >>>> -identifier fn = rule1.fn, local_err, local_err2; >>>> +identifier fn, _errp, local_err, local_err2; >>>> @@ >>>> >>>> - fn(..., Error ** ____, ...) >>>> + fn(..., Error **_errp, ...) >>>> { >>>> ... >>>> Error *local_err = NULL; >>>> >>>> Warnings remain unchanged. >>>> >>>>> +@ script:python @ >>>>> +fn << check2.fn; >>>>> +p1 << check2.p1; >>>>> +p2 << check2.p2; >>>>> +@@ >>>>> + >>>>> +print('Warning: function {} propagates to errp several times in ' >>>>> + 'one control flow: at {}:{} and then at {}:{}'.format( >>>>> + fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line)) >>>>> + >>>>> +// Convert special case with goto separately. >>>>> +// 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: }". >>>>> +@ disable optional_qualifier@ >>>>> +identifier rule1.fn, rule1.local_err, out; >>>> >>>> As explained above, I doubt the need for rule1.fn. We do need >>>> rule1.local_err to avoid unwanted transformations. More of the same >>>> below. >>> >>> Logically, I want to inherit from rule1. So why not to stress it by >>> inheriting >>> fn variable? It's just a correct thing to do. >>> And I hope it helps coccinelle to work more efficiently. >>> >>>> >>>>> +symbol errp; >>>>> +@@ >>>>> + >>>>> + fn(..., Error ** ____, ...) >>>>> + { >>>>> + <... >>>>> +- goto out; >>>>> ++ return; >>>>> + ...> >>>>> +- out: >>>>> +- error_propagate(errp, local_err); >>>>> + } >>>>> + >>>>> +// Convert most of local_err related 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). >>>>> +// >>>>> +// 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; >>>>> +// } >>>>> +@ disable optional_qualifier exists@ >>>>> +identifier rule1.fn, rule1.local_err; >>>>> +expression list args; >>>>> +symbol errp; >>>>> +@@ >>>>> + >>>>> + fn(..., Error ** ____, ...) >>>>> + { >>>>> + <... >>>>> +( >>>>> +- Error *local_err = NULL; >>>>> +| >>>>> + >>>>> +// Convert error clearing functions >>>>> +( >>>>> +- 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); >>>>> +) >>>>> +?- local_err = NULL; >>>>> + >>>>> +| >>>>> +- error_propagate_prepend(errp, local_err, args); >>>>> ++ error_prepend(errp, args); >>>>> +| >>>>> +- error_propagate(errp, local_err); >>>>> +| >>>>> +- &local_err >>>>> ++ errp >>>>> +) >>>>> + ...> >>>>> + } >>>>> + >>>>> +// Convert remaining local_err usage. For example, different kinds of >>>>> +// error checking in if conditionals. We can't merge this into >>>>> +// previous hunk, as this conflicts with other substitutions in it (at >>>>> +// least with "- local_err = NULL"). >>>>> +@ disable optional_qualifier@ >>>>> +identifier rule1.fn, rule1.local_err; >>>>> +symbol errp; >>>>> +@@ >>>>> + >>>>> + fn(..., Error ** ____, ...) >>>>> + { >>>>> + <... >>>>> +- local_err >>>>> ++ *errp >>>>> + ...> >>>>> + } >>>>> + >>>>> +// Always use the same pattern for checking error >>>>> +@ disable optional_qualifier@ >>>>> +identifier rule1.fn; >>>>> +symbol errp; >>>>> +@@ >>>>> + >>>>> + fn(..., Error ** ____, ...) >>>>> + { >>>>> + <... >>>>> +- *errp != NULL >>>>> ++ *errp >>>>> + ...> >>>>> + } >>>>> + >>>>> +// Revert temporary ___ identifier. >>>>> +@ disable optional_qualifier@ >>>>> +identifier rule1.fn; >>>>> +@@ >>>>> + >>>>> + fn(..., Error ** >>>>> +- ____ >>>>> ++ errp >>>>> + , ...) >>>>> + { >>>>> + ... >>>>> + } >>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h >>>>> index 30140d9bfe..56c133520d 100644 >>>>> --- a/include/qapi/error.h >>>>> +++ b/include/qapi/error.h >>>>> @@ -214,6 +214,9 @@ >>>>> * } >>>>> * ... >>>>> * } >>>>> + * >>>>> + * For mass-conversion use script >>>>> + * scripts/coccinelle/auto-propagated-errp.cocci >>>>> */ >>>>> #ifndef ERROR_H >>>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>>> index 857f969aa1..047f1b9714 100644 >>>>> --- a/MAINTAINERS >>>>> +++ b/MAINTAINERS >>>>> @@ -1998,6 +1998,7 @@ F: include/qemu/error-report.h >>>>> F: qapi/error.json >>>>> F: util/error.c >>>>> F: util/qemu-error.c >>>>> +F: scripts/coccinelle/*err*.cocci >>>>> GDB stub >>>>> M: Alex Bennée <alex.bennee@xxxxxxxxxx> >>>> >> >> >> From 42a08c529024337d1b859839c9ce7f797f784555 Mon Sep 17 00:00:00 2001 >> From: Markus Armbruster <armbru@xxxxxxxxxx> >> Date: Fri, 13 Mar 2020 14:27:57 +0100 >> Subject: [PATCH] fixup! scripts: Coccinelle script to use >> ERRP_AUTO_PROPAGATE() >> >> --- >> scripts/coccinelle/auto-propagated-errp.cocci | 37 ++++++++++--------- >> 1 file changed, 20 insertions(+), 17 deletions(-) >> >> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci >> b/scripts/coccinelle/auto-propagated-errp.cocci >> index 7dac2dcfa4..43b0b0e63b 100644 >> --- a/scripts/coccinelle/auto-propagated-errp.cocci >> +++ b/scripts/coccinelle/auto-propagated-errp.cocci >> @@ -136,45 +136,48 @@ symbol errp; >> // Warn several Error * definitions. >> @check1 disable optional_qualifier exists@ >> -identifier fn = rule1.fn, local_err, local_err2; >> +identifier fn, _errp, local_err, local_err2; >> +position p1, p2; > > > Hmm, seems like I forget to define ____ as symbol in my patch Coccinelle defaults to symbol. >> @@ >> - fn(..., Error ** ____, ...) >> + fn(..., Error **_errp, ...) > > Ahmm.. it will break compilation? > > Or, how will it work when _errp defined as meta variable is only in "+..." > line? Should it be symbol instead, or just not defined? Misunderstanding? It's a diff between your .cocci and mine. My version is // Warn several Error * definitions. @check1 disable optional_qualifier exists@ identifier fn, _errp, local_err, local_err2; position p1, p2; @@ fn(..., Error **_errp, ...) { ... Error *local_err = NULL;@p1 ... when any Error *local_err2 = NULL;@p2 ... when any } @ script:python @ fn << check1.fn; p1 << check1.p1; p2 << check1.p2; @@ >> { >> ... >> - Error *local_err = NULL; >> + Error *local_err = NULL;@p1 > > Why to do -/+ here? Nothing changed.. > >> ... when any >> - Error *local_err2 = NULL; >> + Error *local_err2 = NULL;@p2 >> ... when any >> } >> @ script:python @ >> fn << check1.fn; >> +p1 << check1.p1; >> +p2 << check1.p2; >> @@ >> print('Warning: function {} has several definitions of ' >> - 'Error * local variable'.format(fn)) >> + 'Error * local variable: at {}:{} and then at {}:{}'.format( >> + fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line)) >> // Warn several propagations in control flow. >> @check2 disable optional_qualifier exists@ >> -identifier fn = rule1.fn; >> -symbol errp; >> +identifier fn, _errp; >> position p1, p2; >> @@ >> - fn(..., Error ** ____, ...) >> + fn(..., Error **_errp, ...) >> { >> ... >> ( >> - error_propagate_prepend(errp, ...);@p1 >> + error_propagate_prepend(_errp, ...);@p1 >> | >> - error_propagate(errp, ...);@p1 >> + error_propagate(_errp, ...);@p1 >> ) >> ... >> ( >> - error_propagate_prepend(errp, ...);@p2 >> + error_propagate_prepend(_errp, ...);@p2 >> | >> - error_propagate(errp, ...);@p2 >> + error_propagate(_errp, ...);@p2 >> ) > > You change some occurrences of errp to _errp, but not all. It breaks > compilation. > >> ... when any >> } >> @@ -198,7 +201,7 @@ print('Warning: function {} propagates to errp several >> times in ' >> // the same, just without error_propagate() call), coccinelle fails to >> // match this "out: }". >> @ disable optional_qualifier@ >> -identifier rule1.fn, rule1.local_err, out; >> +identifier fn, rule1.local_err, out; > > Hmm. If it improves performance it is strange.. But I can live with this > change. > >> symbol errp; >> @@ >> @@ -239,7 +242,7 @@ symbol errp; >> // *errp = NULL; >> // } >> @ disable optional_qualifier exists@ >> -identifier rule1.fn, rule1.local_err; >> +identifier fn, rule1.local_err; >> expression list args; >> symbol errp; >> @@ >> @@ -287,7 +290,7 @@ symbol errp; >> // previous hunk, as this conflicts with other substitutions in it (at >> // least with "- local_err = NULL"). >> @ disable optional_qualifier@ >> -identifier rule1.fn, rule1.local_err; >> +identifier fn, rule1.local_err; >> symbol errp; >> @@ >> @@ -301,7 +304,7 @@ symbol errp; >> // Always use the same pattern for checking error >> @ disable optional_qualifier@ >> -identifier rule1.fn; >> +identifier fn; >> symbol errp; >> @@ >> @@ -315,7 +318,7 @@ symbol errp; >> // Revert temporary ___ identifier. >> @ disable optional_qualifier@ >> -identifier rule1.fn; >> +identifier fn; >> @@ >> fn(..., Error ** >> I append my hacked up version of auto-propagated-errp.cocci. It produces the same patch as yours for the complete tree. // 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 FILES... // // Note: --max-width 80 is needed because coccinelle default is less // than 80, and without this parameter coccinelle may reindent some // lines which fit into 80 characters but not to coccinelle default, // which in turn produces extra patch hunks for no reason. // Switch unusual Error ** parameter names to errp // (this is necessary to use ERRP_AUTO_PROPAGATE). // // Disable optional_qualifier to skip functions with // "Error *const *errp" parameter. // // Skip functions with "assert(_errp && *_errp)" statement, because // that signals unusual semantics, and the parameter name may well // serve a purpose. (like nbd_iter_channel_error()). // // Skip util/error.c to not touch, for example, error_propagate() and // error_propagate_prepend(). @ 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 ...> ) } // Add invocation of ERRP_AUTO_PROPAGATE to errp-functions where // necessary // // Note, that without "when any" the final "..." does not mach // something matched by previous pattern, i.e. the rule will not match // double error_prepend in control flow like in // vfio_set_irq_signaling(). // // 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). @ disable optional_qualifier exists@ identifier fn, local_err; symbol errp; @@ fn(..., Error **errp, ...) { + ERRP_AUTO_PROPAGATE(); ... when != ERRP_AUTO_PROPAGATE(); ( ( error_append_hint(errp, ...); | error_prepend(errp, ...); | error_vprepend(errp, ...); ) ... when any | Error *local_err = NULL; ... ( error_propagate_prepend(errp, local_err, ...); | error_propagate(errp, local_err); ) ... ) } // Match functions with propagation of local error to errp. // We want to refer these functions in several following rules, but I // don't know a proper way to inherit a function, not just its name // (to not match another functions with same name in following rules). // Not-proper way is as follows: rename errp parameter in functions // header and match it in following rules. Rename it back after all // transformations. // // The simplest case of propagation scheme is single definition of // local_err with at most one error_propagate_prepend or // error_propagate on each control-flow. Still, we want to match more // complex schemes too. We'll warn them with help of further rules. @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); ) ... } // Warn several Error * definitions. @check1 disable optional_qualifier exists@ identifier fn, _errp, local_err, local_err2; position p1, p2; @@ fn(..., Error **_errp, ...) { ... Error *local_err = NULL;@p1 ... when any Error *local_err2 = NULL;@p2 ... when any } @ script:python @ fn << check1.fn; p1 << check1.p1; p2 << check1.p2; @@ print('Warning: function {} has several definitions of ' 'Error * local variable: at {}:{} and then at {}:{}'.format( fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line)) // Warn several propagations in control flow. @check2 disable optional_qualifier exists@ identifier fn, _errp; position p1, p2; @@ fn(..., Error **_errp, ...) { ... ( error_propagate_prepend(_errp, ...);@p1 | error_propagate(_errp, ...);@p1 ) ... ( error_propagate_prepend(_errp, ...);@p2 | error_propagate(_errp, ...);@p2 ) ... when any } @ script:python @ fn << check2.fn; p1 << check2.p1; p2 << check2.p2; @@ print('Warning: function {} propagates to errp several times in ' 'one control flow: at {}:{} and then at {}:{}'.format( fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line)) // Convert special case with goto separately. // 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: }". @ disable optional_qualifier@ identifier fn, rule1.local_err, out; symbol errp; @@ fn(..., Error ** ____, ...) { <... - goto out; + return; ...> - out: - error_propagate(errp, local_err); } // Convert most of local_err related 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). // // 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; // } @ disable optional_qualifier exists@ identifier fn, rule1.local_err; expression list args; symbol errp; @@ fn(..., Error ** ____, ...) { <... ( - Error *local_err = NULL; | // Convert error clearing functions ( - 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); ) ?- local_err = NULL; | - error_propagate_prepend(errp, local_err, args); + error_prepend(errp, args); | - error_propagate(errp, local_err); | - &local_err + errp ) ...> } // Convert remaining local_err usage. For example, different kinds of // error checking in if conditionals. We can't merge this into // previous hunk, as this conflicts with other substitutions in it (at // least with "- local_err = NULL"). @ disable optional_qualifier@ identifier fn, rule1.local_err; symbol errp; @@ fn(..., Error ** ____, ...) { <... - local_err + *errp ...> } // Always use the same pattern for checking error @ disable optional_qualifier@ identifier fn; symbol errp; @@ fn(..., Error ** ____, ...) { <... - *errp != NULL + *errp ...> } // Revert temporary ___ identifier. @ disable optional_qualifier@ identifier fn; @@ fn(..., Error ** - ____ + errp , ...) { ... } _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |