qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGA

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
Date: Fri, 13 Mar 2020 09:38:21 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 
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 <address@hidden> 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 <address@hidden>
---

Cc: Eric Blake <address@hidden>
Cc: Kevin Wolf <address@hidden>
Cc: Max Reitz <address@hidden>
Cc: Greg Kurz <address@hidden>
Cc: Christian Schoenebeck <address@hidden>
Cc: Stefano Stabellini <address@hidden>
Cc: Anthony Perard <address@hidden>
Cc: Paul Durrant <address@hidden>
Cc: Stefan Hajnoczi <address@hidden>
Cc: "Philippe Mathieu-Daudé" <address@hidden>
Cc: Laszlo Ersek <address@hidden>
Cc: Gerd Hoffmann <address@hidden>
Cc: Stefan Berger <address@hidden>
Cc: Markus Armbruster <address@hidden>
Cc: Michael Roth <address@hidden>
Cc: address@hidden
Cc: address@hidden
Cc: address@hidden

  scripts/coccinelle/auto-propagated-errp.cocci | 327 ++++++++++++++++++
  include/qapi/error.h                          |   3 +
  MAINTAINERS                                   |   1 +
  3 files changed, 331 insertions(+)
  create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 0000000000..7dac2dcfa4
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,327 @@
+// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
+//
+// Copyright (c) 2020 Virtuozzo International GmbH.
+//
+// This program is free software; you can redistribute it and/or
+// modify it under the terms of the GNU General Public License as
+// published by the Free Software Foundation; either version 2 of the
+// License, or (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see
+// <http://www.gnu.org/licenses/>.
+//
+// Usage example:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+//  --macro-file scripts/cocci-macro-file.h --in-place \
+//  --no-show-diff --max-width 80 FILES...
+//
+// Note: --max-width 80 is needed because coccinelle default is less
+// than 80, and without this parameter coccinelle may reindent some
+// lines which fit into 80 characters but not to coccinelle default,
+// which in turn produces extra patch hunks for no reason.


This is about unwanted reformatting of parameter lists due to the ___
chaining hack.  --max-width 80 makes that less likely, but not
impossible.

We can search for unwanted reformatting of parameter lists.  I think
grepping diffs for '^\+.*Error \*\*' should do the trick.  For the whole
tree, I get one false positive (not a parameter list), and one hit:

     @@ -388,8 +388,10 @@ static void object_post_init_with_type(O
          }
      }

     -void object_apply_global_props(Object *obj, const GPtrArray *props, Error 
**errp)
     +void object_apply_global_props(Object *obj, const GPtrArray *props,
     +                               Error **errp)
      {
     +    ERRP_AUTO_PROPAGATE();
          int i;

          if (!props) {

Reformatting, but not unwanted.


Yes, I saw it. This line is 81 character length, so it's OK to fix it in one 
hunk with
ERRP_AUTO_PROPAGATE addition even for non-automatic patch.



The --max-width 80 hack is good enough for me.

It does result in slightly long transformed lines, e.g. this one in
replication.c:

     @@ -113,7 +113,7 @@ static int replication_open(BlockDriverS
              s->mode = REPLICATION_MODE_PRIMARY;
              top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
              if (top_id) {
     -            error_setg(&local_err, "The primary side does not support option 
top-id");
     +            error_setg(errp, "The primary side does not support option 
top-id");
                  goto fail;
              }
          } else if (!strcmp(mode, "secondary")) {

v8 did break this line (that's how I found it).  However, v9 still
shortens the line, just not below the target.  All your + lines look
quite unlikely to lengthen lines.  Let's not worry about this.


+// Switch unusual Error ** parameter names to errp
+// (this is necessary to use ERRP_AUTO_PROPAGATE).
+//
+// Disable optional_qualifier to skip functions with
+// "Error *const *errp" parameter.
+//
+// Skip functions with "assert(_errp && *_errp)" statement, because
+// that signals unusual semantics, and the parameter name may well
+// serve a purpose. (like nbd_iter_channel_error()).
+//
+// Skip util/error.c to not touch, for example, error_propagate() and
+// error_propagate_prepend().
+@ depends on !(file in "util/error.c") disable optional_qualifier@
+identifier fn;
+identifier _errp != errp;
+@@
+
+ fn(...,
+-   Error **_errp
++   Error **errp
+    ,...)
+ {
+(
+     ... when != assert(_errp && *_errp)
+&
+     <...
+-    _errp
++    errp
+     ...>
+)
+ }
+
+// Add invocation of ERRP_AUTO_PROPAGATE to errp-functions where
+// necessary
+//
+// Note, that without "when any" the final "..." does not mach
+// something matched by previous pattern, i.e. the rule will not match
+// double error_prepend in control flow like in
+// vfio_set_irq_signaling().
+//
+// Note, "exists" says that we want apply rule even if it matches not
+// on all possible control flows (otherwise, it will not match
+// standard pattern when error_propagate() call is in if branch).
+@ disable optional_qualifier exists@
+identifier fn, local_err;
+symbol errp;
+@@
+
+ fn(..., Error **errp, ...)
+ {
++   ERRP_AUTO_PROPAGATE();
+    ...  when != ERRP_AUTO_PROPAGATE();
+(
+(
+    error_append_hint(errp, ...);
+|
+    error_prepend(errp, ...);
+|
+    error_vprepend(errp, ...);
+)
+    ... when any
+|
+    Error *local_err = NULL;
+    ...
+(
+    error_propagate_prepend(errp, local_err, ...);
+|
+    error_propagate(errp, local_err);
+)
+    ...
+)
+ }
+
+
+// Match functions with propagation of local error to errp.
+// We want to refer these functions in several following rules, but I
+// don't know a proper way to inherit a function, not just its name
+// (to not match another functions with same name in following rules).
+// Not-proper way is as follows: rename errp parameter in functions
+// header and match it in following rules. Rename it back after all
+// transformations.
+//
+// The simplest case of propagation scheme is single definition of
+// local_err with at most one error_propagate_prepend or
+// error_propagate on each control-flow. Still, we want to match more
+// complex schemes too. We'll warn them with help of further rules.


I think what we actually want is to examine instances of this pattern to
figure out whether and how we want to transform them.  Perhaps:

     // The common case is a single definition of local_err with at most one
     // error_propagate_prepend() or error_propagate() on each control-flow
     // path. Instances of this case we convert with this script. Functions


For me, sounds a bit like "other things we don't convert".
Actually we convert other things too.


     // with multiple definitions or propagates we want to examine
     // manually. Later rules emit warnings to guide us to them.


+@rule1 disable optional_qualifier exists@
+identifier fn, local_err;
+symbol errp;
+@@
+
+ fn(..., Error **
+-    errp
++    ____
+    , ...)
+ {
+     ...
+     Error *local_err = NULL;
+     ...
+(
+     error_propagate_prepend(errp, local_err, ...);
+|
+     error_propagate(errp, local_err);
+)
+     ...
+ }
+
+
+// Warn several Error * definitions.
+@check1 disable optional_qualifier exists@
+identifier fn = rule1.fn, local_err, local_err2;


Elsewhere, you use just rule.fn instead of fn = rule1.fn.  Any
particular reason for the difference?


I didn't find other way to ref check1.fn in next python rule. It just don't
work if I write here just rule1.fn.



With the ___ chaining hack, I doubt we still need "= rule1.fn" or
"rule1.fn".  If I replace "fn = rule1.fn" and "rule.fn" by just "fn"
everywhere, then apply the script to the complete tree, I get the same
result.


I think, it's more efficient to reuse names from previous rules. I think it 
should
work faster (more information, less extra matching).




+@@
+
+ 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))
+
+// Warn several propagations in control flow.
+@check2 disable optional_qualifier exists@
+identifier fn = rule1.fn;
+symbol errp;
+position p1, p2;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+     ...
+(
+     error_propagate_prepend(errp, ...);@p1
+|
+     error_propagate(errp, ...);@p1
+)
+     ...
+(
+     error_propagate_prepend(errp, ...);@p2
+|
+     error_propagate(errp, ...);@p2
+)
+     ... when any
+ }
+


Hmm, we don't catch the example I used in review of v8:

     extern foo(int, Error **);
     extern bar(int, Error **);

     void frob(Error **errp)
     {
         Error *local_err = NULL;
         int arg;

         foo(arg, errp);
         bar(arg, &local_err);
         error_propagate(errp, local_err);
         bar(arg + 1, &local_err);
         error_propagate(errp, local_err);
     }

I believe this is because rule1 does not match here.


Yes, rule1 wants at least one code flow with non-doubled propagation.



If I change the rule as follows, it catches the example:

     @@ -157,24 +157,23 @@ print('Warning: function {} has several definitions 
of '

      // Warn several propagations in control flow.
      @check2 disable optional_qualifier exists@
     -identifier fn = rule1.fn;
     -symbol errp;
     +identifier fn, _errp;
      position p1, p2;
      @@

     - fn(..., Error ** ____, ...)
     + fn(..., Error **_errp, ...)
       {
           ...
      (
     -     error_propagate_prepend(errp, ...);@p1
     +     error_propagate_prepend(_errp, ...);@p1
      |
     -     error_propagate(errp, ...);@p1
     +     error_propagate(_errp, ...);@p1
      )
           ...
      (
     -     error_propagate_prepend(errp, ...);@p2
     +     error_propagate_prepend(_errp, ...);@p2
      |
     -     error_propagate(errp, ...);@p2
     +     error_propagate(_errp, ...);@p2
      )
           ... when any
       }

To my mild surprise, it still doesn't find anything in our tree.

Should we decouple the previous rule from rule1, too?  I tested the
following on the whole tree:


I don't think so. Why to check what we are not going to convert? If we want
to check side things, it's better to do it in other coccinelle script..



     @@ -136,10 +136,10 @@ symbol errp;

      // Warn several Error * definitions.
      @check1 disable optional_qualifier exists@
     -identifier fn = rule1.fn, local_err, local_err2;
     +identifier fn, _errp, local_err, local_err2;
      @@

     - fn(..., Error ** ____, ...)
     + fn(..., Error **_errp, ...)
       {
           ...
           Error *local_err = NULL;

Warnings remain unchanged.


+@ script:python @
+fn << check2.fn;
+p1 << check2.p1;
+p2 << check2.p2;
+@@
+
+print('Warning: function {} propagates to errp several times in '
+      'one control flow: at {}:{} and then at {}:{}'.format(
+          fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line))
+
+// Convert special case with goto separately.
+// I tried merging this into the following rule the obvious way, but
+// it made Coccinelle hang on block.c
+//
+// Note interesting thing: if we don't do it here, and try to fixup
+// "out: }" things later after all transformations (the rule will be
+// the same, just without error_propagate() call), coccinelle fails to
+// match this "out: }".
+@ disable optional_qualifier@
+identifier rule1.fn, rule1.local_err, out;


As explained above, I doubt the need for rule1.fn.  We do need
rule1.local_err to avoid unwanted transformations.  More of the same
below.


Logically, I want to inherit from rule1. So why not to stress it by inheriting
fn variable? It's just a correct thing to do.
And I hope it helps coccinelle to work more efficiently.




+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+     <...
+-    goto out;
++    return;
+     ...>
+- out:
+-    error_propagate(errp, local_err);
+ }
+
+// Convert most of local_err related stuff.
+//
+// Note, that we update everything related to matched by rule1
+// function name and local_err name. We may match something not
+// related to the pattern matched by rule1. For example, local_err may
+// be defined with the same name in different blocks inside one
+// function, and in one block follow the propagation pattern and in
+// other block doesn't. Or we may have several functions with the same
+// name (for different configurations).
+//
+// Note also that errp-cleaning functions
+//   error_free_errp
+//   error_report_errp
+//   error_reportf_errp
+//   warn_report_errp
+//   warn_reportf_errp
+// are not yet implemented. They must call corresponding Error* -
+// freeing function and then set *errp to NULL, to avoid further
+// propagation to original errp (consider ERRP_AUTO_PROPAGATE in use).
+// For example, error_free_errp may look like this:
+//
+//    void error_free_errp(Error **errp)
+//    {
+//        error_free(*errp);
+//        *errp = NULL;
+//    }
+@ disable optional_qualifier exists@
+identifier rule1.fn, rule1.local_err;
+expression list args;
+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+     <...
+(
+-    Error *local_err = NULL;
+|
+
+// Convert error clearing functions
+(
+-    error_free(local_err);
++    error_free_errp(errp);
+|
+-    error_report_err(local_err);
++    error_report_errp(errp);
+|
+-    error_reportf_err(local_err, args);
++    error_reportf_errp(errp, args);
+|
+-    warn_report_err(local_err);
++    warn_report_errp(errp);
+|
+-    warn_reportf_err(local_err, args);
++    warn_reportf_errp(errp, args);
+)
+?-    local_err = NULL;
+
+|
+-    error_propagate_prepend(errp, local_err, args);
++    error_prepend(errp, args);
+|
+-    error_propagate(errp, local_err);
+|
+-    &local_err
++    errp
+)
+     ...>
+ }
+
+// Convert remaining local_err usage. For example, different kinds of
+// error checking in if conditionals. We can't merge this into
+// previous hunk, as this conflicts with other substitutions in it (at
+// least with "- local_err = NULL").
+@ disable optional_qualifier@
+identifier rule1.fn, rule1.local_err;
+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+     <...
+-    local_err
++    *errp
+     ...>
+ }
+
+// Always use the same pattern for checking error
+@ disable optional_qualifier@
+identifier rule1.fn;
+symbol errp;
+@@
+
+ fn(..., Error ** ____, ...)
+ {
+     <...
+-    *errp != NULL
++    *errp
+     ...>
+ }
+
+// Revert temporary ___ identifier.
+@ disable optional_qualifier@
+identifier rule1.fn;
+@@
+
+ fn(..., Error **
+-   ____
++   errp
+    , ...)
+ {
+     ...
+ }
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 30140d9bfe..56c133520d 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -214,6 +214,9 @@
   *         }
   *         ...
   *     }
+ *
+ * For mass-conversion use script
+ *   scripts/coccinelle/auto-propagated-errp.cocci
   */
#ifndef ERROR_H 
diff --git a/MAINTAINERS b/MAINTAINERS
index 857f969aa1..047f1b9714 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1998,6 +1998,7 @@ F: include/qemu/error-report.h
  F: qapi/error.json
  F: util/error.c
  F: util/qemu-error.c
+F: scripts/coccinelle/*err*.cocci
GDB stub 
  M: Alex Bennée <address@hidden>





--
Best regards,
Vladimir
reply via email to
[Prev in Thread] Current Thread [Next in Thread]