Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 20.03.2020 16:58, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
[...]
>>> I will not be surprised, if we missed some more interesting cases :)
>>> But we should proceed. What is our plan? Will you queue v10 for 5.1?
>>
>> v10's PATCH 1+2 look ready.  The error.h comment update could perhaps
>> use some polish; I've focused my attention elsewhere.
>>
>> PATCH 8-9 are generated.  They should never be rebased, always be
>> regenerated.  We compare regenerated patches to posted ones to make sure
>> they are still sane, and the R-bys are still valid.  I can take care of
>> the comparing.
>>
>> I'd like to have a pull request ready when the tree reopens for general
>> development.  Let's use the time until then to get more generated
>> patches out for review.
>>
>> If I queue up patches in my tree, we shift the responsibility for
>> regenerating patches from you to me, and create a coordination issue:
>> you'll want to base patch submissions on the branch I use to queue this
>> work, and that's going to be awkward when I rebase / regenerate that
>> branch.  I think it's simpler to queue up in your tree until we're ready
>> for a pull request.
>>
>> When you post more patches, use
>>
>>  Based-on: <20200317151625.20797-1-vsement...@virtuozzo.com>
>>
>> so that Patchew applies them on top of this series.  Hmm, probably won't
>> do, as PATCH 9 already conflicts.
>>
>> You could instead repost PATCH 1+2 with each batch.  I hope that's not
>> too confusing.
>>
>> I trust you'll keep providing a tag reviewers can pull.
>>
>> I suggest to ask maintainers to leave merging these patches to me, in
>> cover letters.
>>
>> Makes sense?
>>
>
> Hmm.
>
> I remember what Kevin said about freeze period: maintainers will queue
> a lot of patches in their "next" branches, and send pull requests at start
> of next developing period. This highly possible will drop r-bs I can get now.
> And reviewers will have to review twice.
>
> And for the same reason, it's bad idea to queue in your branch a lot of 
> patches
> from different subsystems during freeze.
>
> So, just postpone this all up to next development phase?

Okay.  I hope we can process generated patches at a brisk pace then.

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

20.03.2020 16:58, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


19.03.2020 13:45, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:

[...]

So, understanding that there no such cases in the whole tree, and even
if your patch works faster on the whole tree, I still don't want to
drop inheritance, because it's just a correct thing to do. Yes, we've
added  helper. It helps to avoid some problems. Pair-inheritance
helps to avoid another problems. I understand, that there still may
other, not-covered problems, but better to be as safe as possible. And
inheritance here is native and correct thing to do, even with our 
additional helper. What do you think?


I wouldn't call it correct.  It's still unreliable, but less so than
without the function name constraint.  That makes it less wrong.


Agree.



100% reliable would be nice, but not at any cost.  Something we're
reasonably confident to get right should be good enough.

To be confident, we need to understand the script's limitations, and how
to compensate for them.  I figure we do now.  You too?



I will not be surprised, if we missed some more interesting cases :)
But we should proceed. What is our plan? Will you queue v10 for 5.1?


v10's PATCH 1+2 look ready.  The error.h comment update could perhaps
use some polish; I've focused my attention elsewhere.

PATCH 8-9 are generated.  They should never be rebased, always be
regenerated.  We compare regenerated patches to posted ones to make sure
they are still sane, and the R-bys are still valid.  I can take care of
the comparing.

I'd like to have a pull request ready when the tree reopens for general
development.  Let's use the time until then to get more generated
patches out for review.

If I queue up patches in my tree, we shift the responsibility for
regenerating patches from you to me, and create a coordination issue:
you'll want to base patch submissions on the branch I use to queue this
work, and that's going to be awkward when I rebase / regenerate that
branch.  I think it's simpler to queue up in your tree until we're ready
for a pull request.

When you post more patches, use

 Based-on: <20200317151625.20797-1-vsement...@virtuozzo.com>

so that Patchew applies them on top of this series.  Hmm, probably won't
do, as PATCH 9 already conflicts.

You could instead repost PATCH 1+2 with each batch.  I hope that's not
too confusing.

I trust you'll keep providing a tag reviewers can pull.

I suggest to ask maintainers to leave merging these patches to me, in
cover letters.

Makes sense?



Hmm.

I remember what Kevin said about freeze period: maintainers will queue
a lot of patches in their "next" branches, and send pull requests at start
of next developing period. This highly possible will drop r-bs I can get now.
And reviewers will have to review twice.

And for the same reason, it's bad idea to queue in your branch a lot of patches
from different subsystems during freeze.

So, just postpone this all up to next development phase?


--
Best regards,
Vladimir

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 19.03.2020 13:45, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
[...]
>>> So, understanding that there no such cases in the whole tree, and even
>>> if your patch works faster on the whole tree, I still don't want to
>>> drop inheritance, because it's just a correct thing to do. Yes, we've
>>> added  helper. It helps to avoid some problems. Pair-inheritance
>>> helps to avoid another problems. I understand, that there still may
>>> other, not-covered problems, but better to be as safe as possible. And
>>> inheritance here is native and correct thing to do, even with our 
>>> additional helper. What do you think?
>>
>> I wouldn't call it correct.  It's still unreliable, but less so than
>> without the function name constraint.  That makes it less wrong.
>
> Agree.
>
>>
>> 100% reliable would be nice, but not at any cost.  Something we're
>> reasonably confident to get right should be good enough.
>>
>> To be confident, we need to understand the script's limitations, and how
>> to compensate for them.  I figure we do now.  You too?
>>
>
> I will not be surprised, if we missed some more interesting cases :)
> But we should proceed. What is our plan? Will you queue v10 for 5.1?

v10's PATCH 1+2 look ready.  The error.h comment update could perhaps
use some polish; I've focused my attention elsewhere.

PATCH 8-9 are generated.  They should never be rebased, always be
regenerated.  We compare regenerated patches to posted ones to make sure
they are still sane, and the R-bys are still valid.  I can take care of
the comparing.

I'd like to have a pull request ready when the tree reopens for general
development.  Let's use the time until then to get more generated
patches out for review.

If I queue up patches in my tree, we shift the responsibility for
regenerating patches from you to me, and create a coordination issue:
you'll want to base patch submissions on the branch I use to queue this
work, and that's going to be awkward when I rebase / regenerate that
branch.  I think it's simpler to queue up in your tree until we're ready
for a pull request.

When you post more patches, use

Based-on: <20200317151625.20797-1-vsement...@virtuozzo.com>

so that Patchew applies them on top of this series.  Hmm, probably won't
do, as PATCH 9 already conflicts.

You could instead repost PATCH 1+2 with each batch.  I hope that's not
too confusing.

I trust you'll keep providing a tag reviewers can pull.

I suggest to ask maintainers to leave merging these patches to me, in
cover letters.

Makes sense?

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

19.03.2020 13:45, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


17.03.2020 13:39, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


16.03.2020 11:21, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


On 14.03.2020 00:54, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


13.03.2020 18:42, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  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  writes:

[...]

+@@
+
+ 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.


Hmm, yes.. We can write "Warn several Error * definitions in _one_
control flow (it's not so trivial to match _any_ case with several
definitions with coccinelle)" or something like this.


Ha, "trivial" reminds me of a story.  The math professor, after having
spent a good chunk of his lecture developing a proof on the blackboad
turns to the audience to explain why this little part doesn't require
proof with the words familiar to any math student "and this is trivial."
Pause, puzzled look...  "Is it trivial?"  Pause, storms out of the
lecture hall.  A minute or three pass.  Professor comes back beaming,
"it is trivial!", and proceeds with the proof.

My point is: it might be trivial with Coccinelle once you know how to do
it.  We don't.

Suggest "(can't figure out how to match several definitions regardless
of control flow)".


Wrong too, because I can:) for example, chaining two rules, catching the
positions of definition and check that they are different.. Or, some
cheating with python script.. That's why I wrote "not trivial",

So, most correct would be "(can't figure out how to simply match several 
definitions regardless

of control flow)".


Works for me.


But again, coccinelle is for matching control flows, so its probably impossible 
to match such thing..

[...]

OK, I almost OK with it, the only thing I doubt a bit is the following:

We want to keep rule1.local_err inheritance to keep connection with
local_err definition.


Yes.


Interesting, when we have both rule1.fn and rule1.local_err inherited,
do we inherit them in separate (i.e. all possible combinations of fn
and local_err symbols from rule1) or do we inherit a pair, i.e. only
fn/local_err pairs, found by rule1? If the latter is correct, that
with your script we loss this pair inheritance, and go to all possible
combinations of fn and local_err from rule1, possibly adding some wrong
conversion (OK, you've checked that no such cases in current code tree).


The chaining "identifier rule1.FOO" is by name.  It's reliable only as
long as there is exactly one instance of the name.

We already discussed the case of the function name: if there are two
instances of foo(), and rule1 matches only one of them, then we
nevertheless apply the rules chained to rule1 to both.  Because that can
be wrong, you came up with the ___ trick, which chains reliably.

The same issue exists with the variable name: if there are two instances
of @local_err, and rule1 matches only one of them, then we nevertheless
apply the rules chained to rule1 to both.  Can also be wrong.

What are the conditions for "wrong"?

Because the ___ chaining is reliable, we know rule1 matched the
function, i.e. it has a parameter Error **errp, and it has a automatic
variable Error *local_err = NULL.

We're good as long as *all* identifiers @local_err in this function are
declared that way.  This seems quite likely.  It's not certain, though.

Since nested declarations of Error ** variables are rare, we can rely on
review to ensure we transform these functions correctly.


So, dropping inheritance in check-rules makes sence, as it may match
(and warn) more interesting cases.

But for other rules, I'd prefere to be safer, and explictly inherit all
actually inherited iden

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 17.03.2020 13:39, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> 16.03.2020 11:21, Markus Armbruster wrote:
 Vladimir Sementsov-Ogievskiy  writes:

> On 14.03.2020 00:54, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> 13.03.2020 18:42, Markus Armbruster wrote:
 Vladimir Sementsov-Ogievskiy  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  writes:
>> [...]
>>> +@@
>>> +
>>> + 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.
>
> Hmm, yes.. We can write "Warn several Error * definitions in _one_
> control flow (it's not so trivial to match _any_ case with several
> definitions with coccinelle)" or something like this.

 Ha, "trivial" reminds me of a story.  The math professor, after having
 spent a good chunk of his lecture developing a proof on the blackboad
 turns to the audience to explain why this little part doesn't require
 proof with the words familiar to any math student "and this is trivial."
 Pause, puzzled look...  "Is it trivial?"  Pause, storms out of the
 lecture hall.  A minute or three pass.  Professor comes back beaming,
 "it is trivial!", and proceeds with the proof.

 My point is: it might be trivial with Coccinelle once you know how to do
 it.  We don't.

 Suggest "(can't figure out how to match several definitions regardless
 of control flow)".
>>>
>>> Wrong too, because I can:) for example, chaining two rules, catching the
>>> positions of definition and check that they are different.. Or, some
>>> cheating with python script.. That's why I wrote "not trivial",
>>>
>>> So, most correct would be "(can't figure out how to simply match several 
>>> definitions regardless
 of control flow)".
>>
>> Works for me.
>>
>>> But again, coccinelle is for matching control flows, so its probably 
>>> impossible to match such thing..
>> [...]
> OK, I almost OK with it, the only thing I doubt a bit is the following:
>
> We want to keep rule1.local_err inheritance to keep connection with
> local_err definition.

 Yes.

> Interesting, when we have both rule1.fn and rule1.local_err inherited,
> do we inherit them in separate (i.e. all possible combinations of fn
> and local_err symbols from rule1) or do we inherit a pair, i.e. only
> fn/local_err pairs, found by rule1? If the latter is correct, that
> with your script we loss this pair inheritance, and go to all possible
> combinations of fn and local_err from rule1, possibly adding some wrong
> conversion (OK, you've checked that no such cases in current code tree).

 The chaining "identifier rule1.FOO" is by name.  It's reliable only as
 long as there is exactly one instance of the name.

 We already discussed the case of the function name: if there are two
 instances of foo(), and rule1 matches only one of them, then we
 nevertheless apply the rules chained to rule1 to both.  Because that can
 be wrong, you came up with the ___ trick, which chains reliably.

 The same issue exists with the variable name: if there are two instances
 of @local_err, and rule1 matches only one of them, then we nevertheless
 apply the rules chained to rule1 to both.  Can also be wrong.

 What are the conditions for "

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 14.03.2020 0:54, Markus Armbruster wrote:
[...]
>> // 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, ...)
>
> Hmm, for this to work, we should move the rule above rule1, because now 
> paramter
> definition is different from it usage in the function body.

I think you're right.

[...]

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

14.03.2020 0:54, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


13.03.2020 18:42, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  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  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 
---

Cc: Eric Blake 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: Stefan Hajnoczi 
Cc: "Philippe Mathieu-Daudé" 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Stefan Berger 
Cc: Markus Armbruster 
Cc: Michael Roth 
Cc: qemu-de...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org

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 00..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
+// .
+//
+// 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, an

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

17.03.2020 13:39, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


16.03.2020 11:21, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


On 14.03.2020 00:54, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


13.03.2020 18:42, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  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  writes:

[...]

+@@
+
+ 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.


Hmm, yes.. We can write "Warn several Error * definitions in _one_
control flow (it's not so trivial to match _any_ case with several
definitions with coccinelle)" or something like this.


Ha, "trivial" reminds me of a story.  The math professor, after having
spent a good chunk of his lecture developing a proof on the blackboad
turns to the audience to explain why this little part doesn't require
proof with the words familiar to any math student "and this is trivial."
Pause, puzzled look...  "Is it trivial?"  Pause, storms out of the
lecture hall.  A minute or three pass.  Professor comes back beaming,
"it is trivial!", and proceeds with the proof.

My point is: it might be trivial with Coccinelle once you know how to do
it.  We don't.

Suggest "(can't figure out how to match several definitions regardless
of control flow)".


Wrong too, because I can:) for example, chaining two rules, catching the
positions of definition and check that they are different.. Or, some
cheating with python script.. That's why I wrote "not trivial",

So, most correct would be "(can't figure out how to simply match several 
definitions regardless

of control flow)".


Works for me.


But again, coccinelle is for matching control flows, so its probably impossible 
to match such thing..

[...]

OK, I almost OK with it, the only thing I doubt a bit is the following:

We want to keep rule1.local_err inheritance to keep connection with
local_err definition.


Yes.


Interesting, when we have both rule1.fn and rule1.local_err inherited,
do we inherit them in separate (i.e. all possible combinations of fn
and local_err symbols from rule1) or do we inherit a pair, i.e. only
fn/local_err pairs, found by rule1? If the latter is correct, that
with your script we loss this pair inheritance, and go to all possible
combinations of fn and local_err from rule1, possibly adding some wrong
conversion (OK, you've checked that no such cases in current code tree).


The chaining "identifier rule1.FOO" is by name.  It's reliable only as
long as there is exactly one instance of the name.

We already discussed the case of the function name: if there are two
instances of foo(), and rule1 matches only one of them, then we
nevertheless apply the rules chained to rule1 to both.  Because that can
be wrong, you came up with the ___ trick, which chains reliably.

The same issue exists with the variable name: if there are two instances
of @local_err, and rule1 matches only one of them, then we nevertheless
apply the rules chained to rule1 to both.  Can also be wrong.

What are the conditions for "wrong"?

Because the ___ chaining is reliable, we know rule1 matched the
function, i.e. it has a parameter Error **errp, and it has a automatic
variable Error *local_err = NULL.

We're good as long as *all* identifiers @local_err in this function are
declared that way.  This seems quite likely.  It's not certain, though.

Since nested declarations of Error ** variables are rare, we can rely on
review to ensure we transform these functions correctly.


So, dropping inheritance in check-rules makes sence, as it may match
(and warn) more interesting cases.

But for other rules, I'd prefere to be safer, and explictly inherit all
actually inherited identifiers..


I still can't see what chaining by function name in addition to the ___
chaining buys

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 16.03.2020 11:21, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> On 14.03.2020 00:54, Markus Armbruster wrote:
 Vladimir Sementsov-Ogievskiy  writes:

> 13.03.2020 18:42, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  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  writes:
[...]
> +@@
> +
> + 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.
>>>
>>> Hmm, yes.. We can write "Warn several Error * definitions in _one_
>>> control flow (it's not so trivial to match _any_ case with several
>>> definitions with coccinelle)" or something like this.
>>
>> Ha, "trivial" reminds me of a story.  The math professor, after having
>> spent a good chunk of his lecture developing a proof on the blackboad
>> turns to the audience to explain why this little part doesn't require
>> proof with the words familiar to any math student "and this is trivial."
>> Pause, puzzled look...  "Is it trivial?"  Pause, storms out of the
>> lecture hall.  A minute or three pass.  Professor comes back beaming,
>> "it is trivial!", and proceeds with the proof.
>>
>> My point is: it might be trivial with Coccinelle once you know how to do
>> it.  We don't.
>>
>> Suggest "(can't figure out how to match several definitions regardless
>> of control flow)".
>
> Wrong too, because I can:) for example, chaining two rules, catching the
> positions of definition and check that they are different.. Or, some
> cheating with python script.. That's why I wrote "not trivial",
>
> So, most correct would be "(can't figure out how to simply match several 
> definitions regardless
>> of control flow)".

Works for me.

> But again, coccinelle is for matching control flows, so its probably 
> impossible to match such thing..
[...]
>>> OK, I almost OK with it, the only thing I doubt a bit is the following:
>>>
>>> We want to keep rule1.local_err inheritance to keep connection with
>>> local_err definition.
>>
>> Yes.
>>
>>> Interesting, when we have both rule1.fn and rule1.local_err inherited,
>>> do we inherit them in separate (i.e. all possible combinations of fn
>>> and local_err symbols from rule1) or do we inherit a pair, i.e. only
>>> fn/local_err pairs, found by rule1? If the latter is correct, that
>>> with your script we loss this pair inheritance, and go to all possible
>>> combinations of fn and local_err from rule1, possibly adding some wrong
>>> conversion (OK, you've checked that no such cases in current code tree).
>>
>> The chaining "identifier rule1.FOO" is by name.  It's reliable only as
>> long as there is exactly one instance of the name.
>>
>> We already discussed the case of the function name: if there are two
>> instances of foo(), and rule1 matches only one of them, then we
>> nevertheless apply the rules chained to rule1 to both.  Because that can
>> be wrong, you came up with the ___ trick, which chains reliably.
>>
>> The same issue exists with the variable name: if there are two instances
>> of @local_err, and rule1 matches only one of them, then we nevertheless
>> apply the rules chained to rule1 to both.  Can also be wrong.
>>
>> What are the conditions for "wrong"?
>>
>> Because the ___ chaining is reliable, we know rule1 matched the
>> function, i.e. it has a parameter Error **errp, and it has a automatic
>> variable Error *local_err = NULL.
>>
>> We're good as long as *all* identifiers @local_err in this function are
>> declared that way.  This seems quite likely.  It's not certain, though.
>>
>>

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

16.03.2020 11:21, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


On 14.03.2020 00:54, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


13.03.2020 18:42, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  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  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 
---

Cc: Eric Blake 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: Stefan Hajnoczi 
Cc: "Philippe Mathieu-Daudé" 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Stefan Berger 
Cc: Markus Armbruster 
Cc: Michael Roth 
Cc: qemu-de...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org

 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 00..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
+// .
+//
+// 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" paramete

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> On 14.03.2020 00:54, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> 13.03.2020 18:42, Markus Armbruster wrote:
 Vladimir Sementsov-Ogievskiy  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  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 
>>> ---
>>>
>>> Cc: Eric Blake 
>>> Cc: Kevin Wolf 
>>> Cc: Max Reitz 
>>> Cc: Greg Kurz 
>>> Cc: Christian Schoenebeck 
>>> Cc: Stefano Stabellini 
>>> Cc: Anthony Perard 
>>> Cc: Paul Durrant 
>>> Cc: Stefan Hajnoczi 
>>> Cc: "Philippe Mathieu-Daudé" 
>>> Cc: Laszlo Ersek 
>>> Cc: Gerd Hoffmann 
>>> Cc: Stefan Berger 
>>> Cc: Markus Armbruster 
>>> Cc: Michael Roth 
>>> Cc: qemu-de...@nongnu.org
>>> Cc: qemu-block@nongnu.org
>>> Cc: xen-de...@lists.xenproject.org
>>>
>>> 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 00..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
>>> +// .
>>> +//
>>> +// 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 

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

On 14.03.2020 00:54, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


13.03.2020 18:42, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  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  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 
---

Cc: Eric Blake 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: Stefan Hajnoczi 
Cc: "Philippe Mathieu-Daudé" 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Stefan Berger 
Cc: Markus Armbruster 
Cc: Michael Roth 
Cc: qemu-de...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org

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 00..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
+// .
+//
+// 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 semant

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Eric Blake  writes:

> On 3/13/20 4:54 PM, Markus Armbruster wrote:
>
>>
>> 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)
>> //
>
>> //
>> // 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.
>
> Do we really need this note?  And/or should we update other Coccinelle
> script examples to mention --max-width?

What makes this Coccinelle script special is its rule chaining hack.  We
want to transform certain functions by applying a sequence of rules.  We
want to chain these rules together, i.e. have subsequent rules match
only where the first rule matches.  We do this by renaming the Error
**errp parameter to ___ in the first rule, and back to errp in the last
rule.  The two renames cancel out, but of course Coccinelle doesn't
special-case that, but does what it always does when it touches long
lines: it wraps them.  This leads to unwanted patch hunks wrapping
formal parameter lists.

Increasing Coccinelle's line width limit just a bit gets rid of almost
all of them.

>> // 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@
>
> The comments are definitely helpful.

Oh boy, they are!

>> 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().
>
> How likely are we that this comment might go stale over time?  But I'm
> not opposed to it.

My plan is to complete the conversion in 5.1.  The script should become
uninteresting soon after.  Comments that risk going stale don't bother
me.

>> //
>> // Note, "exists" says that we want apply rule even if it matches not
>> // on all possible control flows (otherwise, it will not match
>
> s/matches not on/does not match on/
>
>> // 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.
>
> We'll warn for those with the 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 E

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Eric Blake]

On 3/13/20 4:54 PM, Markus Armbruster wrote:



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)
//



//
// 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.


Do we really need this note?  And/or should we update other Coccinelle 
script examples to mention --max-width?




// 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@


The comments are definitely helpful.


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().


How likely are we that this comment might go stale over time?  But I'm 
not opposed to it.



//
// Note, "exists" says that we want apply rule even if it matches not
// on all possible control flows (otherwise, it will not match


s/matches not on/does not match on/


// 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.


We'll warn for those with the 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.


// Warn when there are 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.


// Warn when several propagations are in the 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 {}:{} a

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 13.03.2020 18:42, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  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  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 
> ---
>
> Cc: Eric Blake 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Greg Kurz 
> Cc: Christian Schoenebeck 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Stefan Hajnoczi 
> Cc: "Philippe Mathieu-Daudé" 
> Cc: Laszlo Ersek 
> Cc: Gerd Hoffmann 
> Cc: Stefan Berger 
> Cc: Markus Armbruster 
> Cc: Michael Roth 
> Cc: qemu-de...@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: xen-de...@lists.xenproject.org
>
>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 00..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
> +// .
> +//
> +// 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 fa

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

13.03.2020 18:42, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  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  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 
---

Cc: Eric Blake 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: Stefan Hajnoczi 
Cc: "Philippe Mathieu-Daudé" 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Stefan Berger 
Cc: Markus Armbruster 
Cc: Michael Roth 
Cc: qemu-de...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org

   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 00..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
+// .
+//
+// 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

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  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  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 
>>> ---
>>>
>>> Cc: Eric Blake 
>>> Cc: Kevin Wolf 
>>> Cc: Max Reitz 
>>> Cc: Greg Kurz 
>>> Cc: Christian Schoenebeck 
>>> Cc: Stefano Stabellini 
>>> Cc: Anthony Perard 
>>> Cc: Paul Durrant 
>>> Cc: Stefan Hajnoczi 
>>> Cc: "Philippe Mathieu-Daudé" 
>>> Cc: Laszlo Ersek 
>>> Cc: Gerd Hoffmann 
>>> Cc: Stefan Berger 
>>> Cc: Markus Armbruster 
>>> Cc: Michael Roth 
>>> Cc: qemu-de...@nongnu.org
>>> Cc: qemu-block@nongnu.org
>>> Cc: xen-de...@lists.xenproject.org
>>>
>>>   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 00..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
>>> +// .
>>> +//
>>> +// 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

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 13.03.2020 10:50, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>> [...]
>>> +// Warn several Error * definitions.
>>> +@check1 disable optional_qualifier exists@
>>> +identifier fn = rule1.fn, local_err, local_err2;
>>> +@@
>>> +
>>> + fn(..., Error ** , ...)
>>> + {
>>> + ...
>>> + Error *local_err = NULL;
>>> + ... when any
>>> + Error *local_err2 = NULL;
>>> + ... when any
>>> + }
>>> +
>>> +@ script:python @
>>> +fn << check1.fn;
>>> +@@
>>> +
>>> +print('Warning: function {} has several definitions of '
>>> +  'Error * local variable'.format(fn))
>>
>> Printing the positions like you do in the next rule is useful when
>> examining these warnings.
>
> I decided that searching for Error * definition is simple, and better for
> user to search all definitions by hand (may be more than too).
>
> But understanding control flows is more complex thing and better to help
> user with line positions.
>
> But if you want, we can add them of course. Note, that for some reasons some
> times coccinelle instead of original filename prints something like 
> /tmp/...original-name...
> so it don't look nice and may be a bit misleading.

I noticed when I actually tried mu idea %-}

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

13.03.2020 17:58, Markus Armbruster wrote:

I tried this script on the whole tree.  Observations:

* $ git-diff --shortstat \*.[ch]
333 files changed, 3480 insertions(+), 4586 deletions(-)

* Twelve functions have "several definitions of Error * local variable".

   Eight declare such a variable within a loop.  Reported because
   Coccinelle matches along control flow, not just along text.  Ignore.

   Remaining four:

   * ivshmem_common_realize()

 Two variables (messed up in commit fe44dc91807), should be replaced
 by one.

   * qmp_query_cpu_model_expansion() two times

 Three declarations in separate blocks; two should be replaced by
 &error_abort, one moved to the function block.

   * xen_block_device_destroy()

 Two declarations in seperate blocks; should be replaced by a single
 one.

   Separate manual cleanup patches, ideally applied before running
   Coccinelle to keep Coccinelle's changes as simple and safe as
   possible.  I'll post patches.  Only the one for
   xen_block_device_destroy() affects by this series.

* No function "propagates to errp several times"

   I tested the rule does detect this as advertized by feeding it an
   obvious example.  We're good.

* ERRP_AUTO_PROPAGATE() inserted 744 times, always right at the
   beginning of a function.

* As far as I can tell, all changed functions have ERRP_AUTO_PROPAGATE()
   inserted.  Good.

* Almost 1100 error propagations dropped:error_propagate() removed,
   error_propagate_prepend() replaced by just error_prepend().

* Four error_propagate() are transformed.  Two instances each in
   aspeed_soc_ast2600_realize() and aspeed_soc_realize().  Pattern:

  {
 +ERRP_AUTO_PROPAGATE();
  ...
 -Error *err = NULL, *local_err = NULL;
 +Error *local_err = NULL;
  ...

  object_property_set_T(...,
 -  &err);
 +  errp);
  object_property_set_T(...,
&local_err);
 -error_propagate(&err, local_err);
 -if (err) {
 -error_propagate(errp, err);
 +error_propagate(errp, local_err);
 +if (*errp) {
  return;
  }

   This is what error.h calls "Receive and accumulate multiple errors
   (first one wins)".

   Result:

 ERRP_AUTO_PROPAGATE();
 ...
 Error *local_err = NULL;
 ...

 object_property_set_T(..., errp);
 object_property_set_T(..., &local_err);
 error_propagate(errp, local_err);
 if (*errp) {
 return;
 }

   Could be done without the accumulation:

 ERRP_AUTO_PROPAGATE();
 ...

 object_property_set_T(..., errp);
 if (*errp) {
 return;
 }
 object_property_set_T(..., errp);
 if (*errp) {
 return;
 }

   I find this a bit easier to understand.  Matter of taste.  If we want
   to change to this, do it manually and separately.  I'd do it on top.

* Some 90 propagations remain.

   Some of them could use cleanup, e.g. file_memory_backend_set_pmem(),
   css_clear_io_interrupt().  Out of scope for this series.

   Some move errors around in unusual ways, e.g. in block/nbd.c.  Could
   use review.  Out of scope for this series.

   I spotted three that should be transformed, but aren't:

   - qcrypto_block_luks_store_key()

 I believe g_autoptr() confuses Coccinelle.  Undermines all our
 Coccinelle use, not just this patch.  I think we need to update
 scripts/cocci-macro-file.h for it.

   - armsse_realize()

 Something in this huge function confuses Coccinelle, but I don't
 know what exactly.  If I delete most of it, the error_propagate()
 transforms okay.  If I delete less, Coccinelle hangs.

   - apply_cpu_model()

 Gets transformed fine if I remove the #ifndef CONFIG_USER_ONLY.  I
 have no idea why the #if spooks Coccinelle here, but not elsewhere.

   None of these three affects this series.  No need to hold it back for
   further investigation.

* 30 error_free() and two warn_reportf_err() are transformed.  Patterns:

 -error_free(local_err);
 -local_err = NULL;
 +error_free_errp(errp);

   and

 -error_free(local_err);
 +error_free_errp(errp);

   and

 -warn_report_err(local_err);
 -local_err = NULL;
 +warn_report_errp(errp);

   Good.

* Many error_free(), error_reportf_err() and warn_reportf_err() remain.
   None of them have an argument of the form *errp.  Such arguments would
   have to be reviewed for possible interference with
   ERRP_AUTO_PROPAGATE().

* Almost 700 Error *err = NULL removed.  Almost 600 remain.

* Error usage in rdma.c is questionable / wrong.  Out of scope for this
   series.

As far as I can tell, your Coccinelle script is wor

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

I tried this script on the whole tree.  Observations:

* $ git-diff --shortstat \*.[ch]
   333 files changed, 3480 insertions(+), 4586 deletions(-)

* Twelve functions have "several definitions of Error * local variable".

  Eight declare such a variable within a loop.  Reported because
  Coccinelle matches along control flow, not just along text.  Ignore.

  Remaining four:

  * ivshmem_common_realize()

Two variables (messed up in commit fe44dc91807), should be replaced
by one.

  * qmp_query_cpu_model_expansion() two times

Three declarations in separate blocks; two should be replaced by
&error_abort, one moved to the function block.

  * xen_block_device_destroy()

Two declarations in seperate blocks; should be replaced by a single
one.

  Separate manual cleanup patches, ideally applied before running
  Coccinelle to keep Coccinelle's changes as simple and safe as
  possible.  I'll post patches.  Only the one for
  xen_block_device_destroy() affects by this series.

* No function "propagates to errp several times"

  I tested the rule does detect this as advertized by feeding it an
  obvious example.  We're good.

* ERRP_AUTO_PROPAGATE() inserted 744 times, always right at the
  beginning of a function.

* As far as I can tell, all changed functions have ERRP_AUTO_PROPAGATE()
  inserted.  Good.

* Almost 1100 error propagations dropped:error_propagate() removed,
  error_propagate_prepend() replaced by just error_prepend().

* Four error_propagate() are transformed.  Two instances each in
  aspeed_soc_ast2600_realize() and aspeed_soc_realize().  Pattern:

 {
+ERRP_AUTO_PROPAGATE();
 ...
-Error *err = NULL, *local_err = NULL;
+Error *local_err = NULL;
 ...

 object_property_set_T(..., 
-  &err);
+  errp);
 object_property_set_T(...,
   &local_err);
-error_propagate(&err, local_err);
-if (err) {
-error_propagate(errp, err);
+error_propagate(errp, local_err);
+if (*errp) {
 return;
 }

  This is what error.h calls "Receive and accumulate multiple errors
  (first one wins)".

  Result:

ERRP_AUTO_PROPAGATE();
...
Error *local_err = NULL;
...

object_property_set_T(..., errp);
object_property_set_T(..., &local_err);
error_propagate(errp, local_err);
if (*errp) {
return;
}

  Could be done without the accumulation:

ERRP_AUTO_PROPAGATE();
...

object_property_set_T(..., errp);
if (*errp) {
return;
}
object_property_set_T(..., errp);
if (*errp) {
return;
}

  I find this a bit easier to understand.  Matter of taste.  If we want
  to change to this, do it manually and separately.  I'd do it on top.

* Some 90 propagations remain.

  Some of them could use cleanup, e.g. file_memory_backend_set_pmem(),
  css_clear_io_interrupt().  Out of scope for this series.

  Some move errors around in unusual ways, e.g. in block/nbd.c.  Could
  use review.  Out of scope for this series.

  I spotted three that should be transformed, but aren't:

  - qcrypto_block_luks_store_key()

I believe g_autoptr() confuses Coccinelle.  Undermines all our
Coccinelle use, not just this patch.  I think we need to update
scripts/cocci-macro-file.h for it.

  - armsse_realize()

Something in this huge function confuses Coccinelle, but I don't
know what exactly.  If I delete most of it, the error_propagate()
transforms okay.  If I delete less, Coccinelle hangs.

  - apply_cpu_model()

Gets transformed fine if I remove the #ifndef CONFIG_USER_ONLY.  I
have no idea why the #if spooks Coccinelle here, but not elsewhere.

  None of these three affects this series.  No need to hold it back for
  further investigation.

* 30 error_free() and two warn_reportf_err() are transformed.  Patterns:

-error_free(local_err);
-local_err = NULL;
+error_free_errp(errp);

  and

-error_free(local_err);
+error_free_errp(errp);

  and

-warn_report_err(local_err);
-local_err = NULL;
+warn_report_errp(errp);

  Good.

* Many error_free(), error_reportf_err() and warn_reportf_err() remain.
  None of them have an argument of the form *errp.  Such arguments would
  have to be reviewed for possible interference with
  ERRP_AUTO_PROPAGATE().

* Almost 700 Error *err = NULL removed.  Almost 600 remain.

* Error usage in rdma.c is questionable / wrong.  Out of scope for this
  series.

As far as I can tell, your Coccinelle script is working as intended,
except for three missed error propagations noted above.  We can proceed
with this series regardless, if we want.  I'd prefer t

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

13.03.2020 10:50, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:

[...]

+// Warn several Error * definitions.
+@check1 disable optional_qualifier exists@
+identifier fn = rule1.fn, local_err, local_err2;
+@@
+
+ fn(..., Error ** , ...)
+ {
+ ...
+ Error *local_err = NULL;
+ ... when any
+ Error *local_err2 = NULL;
+ ... when any
+ }
+
+@ script:python @
+fn << check1.fn;
+@@
+
+print('Warning: function {} has several definitions of '
+  'Error * local variable'.format(fn))


Printing the positions like you do in the next rule is useful when
examining these warnings.


I decided that searching for Error * definition is simple, and better for
user to search all definitions by hand (may be more than too).

But understanding control flows is more complex thing and better to help
user with line positions.

But if you want, we can add them of course. Note, that for some reasons some
times coccinelle instead of original filename prints something like 
/tmp/...original-name...
so it don't look nice and may be a bit misleading.




+
+// 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
+ }
+
+@ 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))

[...]




--
Best regards,
Vladimir

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

[...]
> +// Warn several Error * definitions.
> +@check1 disable optional_qualifier exists@
> +identifier fn = rule1.fn, local_err, local_err2;
> +@@
> +
> + fn(..., Error ** , ...)
> + {
> + ...
> + Error *local_err = NULL;
> + ... when any
> + Error *local_err2 = NULL;
> + ... when any
> + }
> +
> +@ script:python @
> +fn << check1.fn;
> +@@
> +
> +print('Warning: function {} has several definitions of '
> +  'Error * local variable'.format(fn))

Printing the positions like you do in the next rule is useful when
examining these warnings.

> +
> +// 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
> + }
> +
> +@ 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))
[...]

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

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  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 
---

Cc: Eric Blake 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: Stefan Hajnoczi 
Cc: "Philippe Mathieu-Daudé" 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Stefan Berger 
Cc: Markus Armbruster 
Cc: Michael Roth 
Cc: qemu-de...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org

  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 00..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
+// .
+//
+// 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.



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"

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

I may have a second look tomorrow with fresher eyes, but let's get this
out now as is.

Vladimir Sementsov-Ogievskiy  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 
> ---
>
> Cc: Eric Blake 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Greg Kurz 
> Cc: Christian Schoenebeck 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Stefan Hajnoczi 
> Cc: "Philippe Mathieu-Daudé" 
> Cc: Laszlo Ersek 
> Cc: Gerd Hoffmann 
> Cc: Stefan Berger 
> Cc: Markus Armbruster 
> Cc: Michael Roth 
> Cc: qemu-de...@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: xen-de...@lists.xenproject.org
>
>  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 00..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
> +// .
> +//
> +// 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.

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