|
[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
Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
> 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]
As our discussion shows, this script is somewhat hard to understand.
That's okay, it solves a somewhat thorny problem, and I don't have
better ideas. But let me state the intended transformations once more,
so I don't get lost in the details.
Motivation:
1. Make error propagation less error-prone and improve stack backtraces
The "receive error in &local_error, propagate to @errp" pattern is
tedious, error-prone, and produces unhelpful stack backtraces with
&error_abort.
ERRP_AUTO_PROPAGATE() removes manual propagation. It additionally
gives us the stack backtraces we want.
2. Improve error messages with &error_fatal
Passing @errp to error_append_hint(), error_prepend() or
error_vprepend() is useless when @errp is &error_fatal.
ERRP_AUTO_PROPAGATE() fixes this by delaying &error_fatal handling
until the automatic propagation.
The intended transformation has three parts:
* Replace declaration of @local_err by ERRP_AUTO_PROPAGATE() where
needed for 1 or 2.
It's needed when we also drop some error propagation from the function
(motivation 1), or the function calls error_append_hint(),
error_prepend() or error_vprepend() (motivation 2). A function could
do both. I'll refer back to this below.
* Drop error_propagate(errp, local_err)
Special case: error_propagate_prepend(errp, local_err, ...) becomes
error_prepend(errp, ...).
Only correct if there is no other such error_propagate() /
error_propagate_prepend() on any path from the here to return.
* Replace remaining use of @local_err by *errp
>>>>> +
>>>>> +// 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...
I'm fine with not touching "strange" patterns. But we do at least in
the example I gave below: we still add ERRP_AUTO_PROPAGATE().
We either avoid that in the Coccinelle script, or we catch it
afterwards. Catching it afterwards should be feasible:
* If we also drop some error propagation from this function: okay.
* If this function call error_append_hint(), error_prepend() or
error_vprepend(): okay.
* Else: unwanted, back out.
Moreover, I'd like us to double-check we really don't want to touch the
things we don't touch. Feels feasible to me, too: after running
Coccinelle, search for unconverted error_append_hint(), error_prepend(),
error_vprepend(), error_propagate_prepend(), error_propagate().
>> 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)...
I'm not sure I understand this sentence.
The aim of my "possible improvement" is to avoid unwanted
ERRP_AUTO_PROPAGATE() in the Coccinelle script. We can instead search
for unwanted ones after the Coccinelle run, and back them out.
>> 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 .."
Something like this, perhaps?
// Convert remaining local_err usage, typically error checking.
// We can't merge this into previous hunk, as this conflicts with other
// substitutions in it (at least with "- local_err = NULL").
>>>>> +// 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
>>>>> + ...>
>>>>> + }
>>>>
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |