Re: [PATCH v8 02/10] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 11.03.2020 17:41, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 11.03.2020 12:38, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> 09.03.2020 12:56, Markus Armbruster wrote:
>>>>>> Suggest
>>>>>>
>>>>>>        scripts: Coccinelle script to use auto-propagated errp
>>>>>>
>>>>>> or
>>>>>>
>>>>>>        scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
>>>>>>
>>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>> [...]
>>>>>>> +// Note, that we update everything related to matched by rule1 function name
>>>>>>> +// and local_err name. We may match something not related to the pattern
>>>>>>> +// matched by rule1. For example, local_err may be defined with the same name
>>>>>>> +// in different blocks inside one function, and in one block follow the
>>>>>>> +// propagation pattern and in other block doesn't. Or we may have several
>>>>>>> +// functions with the same name (for different configurations).
>>>>>>
>>>>>> Context: rule1 matches functions that have all three of
>>>>>>
>>>>>> * an Error **errp parameter
>>>>>>
>>>>>> * an Error *local_err = NULL variable declaration
>>>>>>
>>>>>> * an error_propagate(errp, local_err) or error_propagate_prepend(errp,
>>>>>>      local_err, ...) expression, where @errp is the parameter and
>>>>>>      @local_err is the variable.
>>>>>>
>>>>>> If I understand you correctly, you're pointing out two potential issues:
>>>>>>
>>>>>> 1. This rule can match functions rule1 does not match if there is
>>>>>> another function with the same name that rule1 does match.
>>>>>>
>>>>>> 2. This rule matches in the entire function matched by rule1, even when
>>>>>> parts of that function use a different @errp or @local_err.
>>>>>>
>>>>>> I figure these apply to all rules with identifier rule1.fn, not just
>>>>>> this one.  Correct?
>>>>>>
>>>>>> Regarding 1.  There must be a better way to chain rules together, but I
>>>>>> don't know it.
>>>>>
>>>>> Hmm, what about something like this:
>>>>>
>>>>> @rule1 disable optional_qualifier exists@
>>>>> identifier fn, local_err;
>>>>> symbol errp;
>>>>> @@
>>>>>
>>>>>    fn(..., Error **
>>>>> - errp
>>>>> + ___errp_coccinelle_updating___
>>>>>       , ...)
>>>>>    {
>>>>>        ...
>>>>>        Error *local_err = NULL;
>>>>>        ...
>>>>> (
>>>>>       error_propagate_prepend(errp, local_err, ...);
>>>>> |
>>>>>       error_propagate(errp, local_err);
>>>>> )
>>>>>        ...
>>>>>    }
>>>>>
>>>>>
>>>>> [..]
>>>>>
>>>>> match symbol ___errp_coccinelle_updating___ in following rules in function header
>>>>>
>>>>> [..]
>>>>>
>>>>>
>>>>> @ disable optional_qualifier@
>>>>> identifier fn, local_err;
>>>>> symbol errp;
>>>>> @@
>>>>>
>>>>>    fn(..., Error **
>>>>> - ___errp_coccinelle_updating___
>>>>> + errp
>>>>>       , ...)
>>>>>    {
>>>>>        ...
>>>>>    }
>>>>>
>>>>>
>>>>> - hacky, but seems not more hacky than python detection, and should work better
>>>>
>>>> As simple, forceful and unsubtle as a sledgehammer.  I like it :)
>>>>
>>>
>>>
>>> Hmm, not so simple.
>>>
>>> It leads to reindenting of function header, which is bad.
>>
>> Because ___errp_coccinelle_updating___ is longer than errp, I guess.
>> Try ____?
>
> I'm afraid not. It's because it just adds \n, when I do
>
> ...,
>
> - errp
> + ___errp_coccinelle_updating___
> ,...

I was thinking of something like the appended patch, which in my
(superficial!) testing leaves alone newlines unless lines are long, but
hangs for block.c.  Oh well.


diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
index bff274bd6d..492a4db826 100644
--- a/scripts/coccinelle/auto-propagated-errp.cocci
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -35,12 +35,12 @@
 // error_propagate_prepend().
 @ depends on !(file in "util/error.c") disable optional_qualifier@
 identifier fn;
-identifier _errp != errp;
+identifier _errp;
 @@
 
  fn(...,
 -   Error **_errp
-+   Error **errp
++   Error **____
     ,...)
  {
 (
@@ -48,7 +48,7 @@ identifier _errp != errp;
 &
      <...
 -    _errp
-+    errp
++    ____
      ...>
 )
  }
@@ -63,26 +63,26 @@ identifier _errp != errp;
 // 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, errp;
+identifier fn, local_err;
 @@
 
- fn(..., Error **errp, ...)
+ fn(..., Error **____, ...)
  {
 +   ERRP_AUTO_PROPAGATE();
     ...  when != ERRP_AUTO_PROPAGATE();
 (
-    error_append_hint(errp, ...);
+    error_append_hint(____, ...);
 |
-    error_prepend(errp, ...);
+    error_prepend(____, ...);
 |
-    error_vprepend(errp, ...);
+    error_vprepend(____, ...);
 |
     Error *local_err = NULL;
     ...
 (
-    error_propagate_prepend(errp, local_err, ...);
+    error_propagate_prepend(____, local_err, ...);
 |
-    error_propagate(errp, local_err);
+    error_propagate(____, local_err);
 )
 )
     ... when any
@@ -92,18 +92,17 @@ identifier fn, local_err, errp;
 // Match scenarios with propagation of local error to errp.
 @rule1 disable optional_qualifier exists@
 identifier fn, local_err;
-symbol errp;
 @@
 
- fn(..., Error **errp, ...)
+ fn(..., Error **____, ...)
  {
      ...
      Error *local_err = NULL;
      ...
 (
-    error_propagate_prepend(errp, local_err, ...);
+    error_propagate_prepend(____, local_err, ...);
 |
-    error_propagate(errp, local_err);
+    error_propagate(____, local_err);
 )
      ...
  }
@@ -118,7 +117,6 @@ symbol errp;
 // without error_propagate() call), coccinelle fails to match this "out: }".
 @@
 identifier rule1.fn, rule1.local_err, out;
-symbol errp;
 @@
 
  fn(...)
@@ -128,7 +126,7 @@ symbol errp;
 +    return;
      ...>
 - out:
--    error_propagate(errp, local_err);
+-    error_propagate(____, local_err);
  }
 
 // Convert most of local_err related staff.
@@ -159,7 +157,6 @@ symbol errp;
 @ exists@
 identifier rule1.fn, rule1.local_err;
 expression list args;
-symbol errp;
 @@
 
  fn(...)
@@ -172,30 +169,30 @@ symbol errp;
 // Convert error clearing functions
 (
 -    error_free(local_err);
-+    error_free_errp(errp);
++    error_free_errp(____);
 |
 -    error_report_err(local_err);
-+    error_report_errp(errp);
++    error_report_errp(____);
 |
 -    error_reportf_err(local_err, args);
-+    error_reportf_errp(errp, args);
++    error_reportf_errp(____, args);
 |
 -    warn_report_err(local_err);
-+    warn_report_errp(errp);
++    warn_report_errp(____);
 |
 -    warn_reportf_err(local_err, args);
-+    warn_reportf_errp(errp, args);
++    warn_reportf_errp(____, args);
 )
 ?-    local_err = NULL;
 
 |
--    error_propagate_prepend(errp, local_err, args);
-+    error_prepend(errp, args);
+-    error_propagate_prepend(____, local_err, args);
++    error_prepend(____, args);
 |
--    error_propagate(errp, local_err);
+-    error_propagate(____, local_err);
 |
 -    &local_err
-+    errp
++    ____
 )
      ...>
  }
@@ -205,27 +202,43 @@ symbol errp;
 // conflicts with other substitutions in it (at least with "- local_err = NULL").
 @@
 identifier rule1.fn, rule1.local_err;
-symbol errp;
 @@
 
  fn(...)
  {
      <...
 -    local_err
-+    *errp
++    *____
      ...>
  }
 
 // Always use the same patter for checking error
 @@
 identifier rule1.fn;
-symbol errp;
 @@
 
  fn(...)
  {
      <...
--    *errp != NULL
-+    *errp
+-    *____ != NULL
++    *____
      ...>
  }
+
+@@
+identifier fn;
+symbol errp;
+@@
+
+ fn(...,
+-   Error **____
++   Error **errp
+    ,...)
+ {
+ ...
+ }
+
+@@
+@@
+-____
++errp