[RFC,v5,025/126] scripts: add coccinelle script to use auto propagated errp

Commit Message

Vladimir Sementsov-Ogievskiy Oct. 11, 2019, 4:04 p.m. UTC

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---

CC: Gerd Hoffmann <[e-mail]>
CC: "Gonglei (Arei)" <[e-mail]>
CC: Eduardo Habkost <[e-mail]>
CC: Igor Mammedov <[e-mail]>
CC: Laurent Vivier <[e-mail]>
CC: Amit Shah <[e-mail]>
CC: Kevin Wolf <[e-mail]>
CC: Max Reitz <[e-mail]>
CC: John Snow <[e-mail]>
CC: Ari Sundholm <[e-mail]>
CC: Pavel Dovgalyuk <[e-mail]>
CC: Paolo Bonzini <[e-mail]>
CC: Stefan Hajnoczi <[e-mail]>
CC: Fam Zheng <[e-mail]>
CC: Stefan Weil <[e-mail]>
CC: Ronnie Sahlberg <[e-mail]>
CC: Peter Lieven <[e-mail]>
CC: Eric Blake <[e-mail]>
CC: "Denis V. Lunev" <[e-mail]>
CC: Markus Armbruster <[e-mail]>
CC: Alberto Garcia <[e-mail]>
CC: Jason Dillaman <[e-mail]>
CC: Wen Congyang <[e-mail]>
CC: Xie Changlong <[e-mail]>
CC: Liu Yuan <[e-mail]>
CC: "Richard W.M. Jones" <[e-mail]>
CC: Jeff Cody <[e-mail]>
CC: "Marc-André Lureau" <[e-mail]>
CC: "Daniel P. Berrangé" <[e-mail]>
CC: Richard Henderson <[e-mail]>
CC: Greg Kurz <[e-mail]>
CC: "Michael S. Tsirkin" <[e-mail]>
CC: Marcel Apfelbaum <[e-mail]>
CC: Beniamino Galvani <[e-mail]>
CC: Peter Maydell <[e-mail]>
CC: "Cédric Le Goater" <[e-mail]>
CC: Andrew Jeffery <[e-mail]>
CC: Joel Stanley <[e-mail]>
CC: Andrew Baumann <[e-mail]>
CC: "Philippe Mathieu-Daudé" <[e-mail]>
CC: Antony Pavlov <[e-mail]>
CC: Jean-Christophe Dubois <[e-mail]>
CC: Peter Chubb <[e-mail]>
CC: Subbaraya Sundeep <[e-mail]>
CC: Eric Auger <[e-mail]>
CC: Alistair Francis <[e-mail]>
CC: "Edgar E. Iglesias" <[e-mail]>
CC: Stefano Stabellini <[e-mail]>
CC: Anthony Perard <[e-mail]>
CC: Paul Durrant <[e-mail]>
CC: Paul Burton <[e-mail]>
CC: Aleksandar Rikalo <[e-mail]>
CC: Chris Wulff <[e-mail]>
CC: Marek Vasut <[e-mail]>
CC: David Gibson <[e-mail]>
CC: Cornelia Huck <[e-mail]>
CC: Halil Pasic <[e-mail]>
CC: Christian Borntraeger <[e-mail]>
CC: "Hervé Poussineau" <[e-mail]>
CC: Xiao Guangrong <[e-mail]>
CC: Aurelien Jarno <[e-mail]>
CC: Aleksandar Markovic <[e-mail]>
CC: Mark Cave-Ayland <[e-mail]>
CC: Jason Wang <[e-mail]>
CC: Laszlo Ersek <[e-mail]>
CC: Yuval Shaia <[e-mail]>
CC: Palmer Dabbelt <[e-mail]>
CC: Sagar Karandikar <[e-mail]>
CC: Bastian Koppelmann <[e-mail]>
CC: David Hildenbrand <[e-mail]>
CC: Thomas Huth <[e-mail]>
CC: Eric Farman <[e-mail]>
CC: Matthew Rosato <[e-mail]>
CC: Hannes Reinecke <[e-mail]>
CC: Michael Walle <[e-mail]>
CC: Artyom Tarasenko <[e-mail]>
CC: Stefan Berger <[e-mail]>
CC: Samuel Thibault <[e-mail]>
CC: Alex Williamson <[e-mail]>
CC: Tony Krowiak <[e-mail]>
CC: Pierre Morel <[e-mail]>
CC: Michael Roth <[e-mail]>
CC: Hailiang Zhang <[e-mail]>
CC: Juan Quintela <[e-mail]>
CC: "Dr. David Alan Gilbert" <[e-mail]>
CC: Luigi Rizzo <[e-mail]>
CC: Giuseppe Lettieri <[e-mail]>
CC: Vincenzo Maffione <[e-mail]>
CC: Jan Kiszka <[e-mail]>
CC: Anthony Green <[e-mail]>
CC: Stafford Horne <[e-mail]>
CC: Guan Xuetao <[e-mail]>
CC: Max Filippov <[e-mail]>
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]

 scripts/coccinelle/auto-propagated-errp.cocci | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

Comments

Vladimir Sementsov-Ogievskiy Oct. 14, 2019, 8:19 a.m. UTC | #1

11.10.2019 20:12, Eric Blake wrote:
> On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>> ---
>>
> 
>>   scripts/coccinelle/auto-propagated-errp.cocci | 118 ++++++++++++++++++
>>   1 file changed, 118 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..d9731620aa
>> --- /dev/null
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> 
>> +@rule1@
>> +// Drop local_err
>> +identifier fn, local_err;
>> +symbol errp;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> +     <...
>> +-    Error *local_err = NULL;
>> +     ...>
>> + }
>> +
> 
> So our goal is to automate removal of all local_err (including when it is spelled err)...
> 
>> +@@
>> +// Handle pattern with goto, otherwise we'll finish up
>> +// with labels at function end which will not compile.
>> +identifier rule1.fn;
>> +identifier rule1.local_err;
>> +identifier OUT;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +-    goto OUT;
>> ++    return;
>> +     ...>
>> +- OUT:
>> +-    error_propagate(errp, local_err);
>> + }
>> +
> 
> this dangling label cleanup makes sense
> 
>> +@@
>> +identifier rule1.fn;
>> +identifier rule1.local_err;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    error_free(local_err);
>> +-    local_err = NULL;
>> ++    error_free_errp(errp);
> 
> This does not make sense - error_free_errp() is not defined prior to this series or anywhere in patches 1-24, if I'm reading it correctly.
> 
>> +|
>> +-    error_free(local_err);
>> ++    error_free_errp(errp);
> 
> and again
> 
>> +|
>> +-    error_report_err(local_err);
>> ++    error_report_errp(errp);
>> +|
>> +-    warn_report_err(local_err);
>> ++    warn_report_errp(errp);
>> +|
>> +-    error_propagate_prepend(errp, local_err,
>> ++    error_prepend(errp,
>> +                              ...);
>> +|
>> +-    error_propagate(errp, local_err);
>> +)
>> +     ...>
>> + }
>> +
> 
> It looks like once this script is run, error_propagate_prepend() will have no clients.

No, it still have a bit, when working with error_copy, and/or moving errors from/to structures.

> Is there a non-generated cleanup patch that removes it (and once it is removed, it can also be removed from the .cocci script as no further clients will reappear later)?

Maybe.

> 
> 
>> +@@
>> +identifier rule1.fn;
>> +identifier rule1.local_err;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    &local_err
>> ++    errp
>> +|
>> +-    local_err
>> ++    *errp
>> +)
>> +     ...>
>> + }
>> +
>> +@@
>> +symbol errp;
>> +@@
>> +
>> +- *errp != NULL
>> ++ *errp
>>
> 
> Seems to make sense.
>

Eric Blake Oct. 14, 2019, 2 p.m. UTC | #2

On 10/14/19 3:19 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> +|
>>> +-    error_propagate(errp, local_err);
>>> +)
>>> +     ...>
>>> + }
>>> +
>>
>> It looks like once this script is run, error_propagate_prepend() will have no clients.
> 
> No, it still have a bit, when working with error_copy, and/or moving errors from/to structures.

No public clients. So can we turn it into a static function only used by 
error.c?

> 
>> Is there a non-generated cleanup patch that removes it (and once it is removed, it can also be removed from the .cocci script as no further clients will reappear later)?
> 
> Maybe.
> 

Basically, if we can get error_propagate out of error.h, that's a good 
sign.  But it's not a show-stopper if we can't do it for some legitimate 
reason (such a reason might include that the definition of the 
ERRP_AUTO_PROPAGATE macro is easier to write if error_propagate remains 
in the .h), so we'll just have to see what is possible.

Patch

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 0000000000..d9731620aa
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,118 @@ 
+@rule0@
+// Add invocation to errp-functions where necessary
+identifier fn, local_err;
+symbol errp;
+@@
+
+ fn(..., Error **errp, ...)
+ {
++   ERRP_AUTO_PROPAGATE();
+    <+...
+(
+    error_append_hint(errp, ...);
+|
+    error_prepend(errp, ...);
+|
+    Error *local_err = NULL;
+)
+    ...+>
+ }
+
+@@
+// Drop doubled invocation
+identifier rule0.fn;
+@@
+
+ fn(...)
+{
+-   ERRP_AUTO_PROPAGATE();
+    ERRP_AUTO_PROPAGATE();
+    ...
+}
+
+@rule1@
+// Drop local_err
+identifier fn, local_err;
+symbol errp;
+@@
+
+ fn(..., Error **errp, ...)
+ {
+     <...
+-    Error *local_err = NULL;
+     ...>
+ }
+
+@@
+// Handle pattern with goto, otherwise we'll finish up
+// with labels at function end which will not compile.
+identifier rule1.fn;
+identifier rule1.local_err;
+identifier OUT;
+@@
+
+ fn(...)
+ {
+     <...
+-    goto OUT;
++    return;
+     ...>
+- OUT:
+-    error_propagate(errp, local_err);
+ }
+
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+     <...
+(
+-    error_free(local_err);
+-    local_err = NULL;
++    error_free_errp(errp);
+|
+-    error_free(local_err);
++    error_free_errp(errp);
+|
+-    error_report_err(local_err);
++    error_report_errp(errp);
+|
+-    warn_report_err(local_err);
++    warn_report_errp(errp);
+|
+-    error_propagate_prepend(errp, local_err,
++    error_prepend(errp,
+                              ...);
+|
+-    error_propagate(errp, local_err);
+)
+     ...>
+ }
+
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+     <...
+(
+-    &local_err
++    errp
+|
+-    local_err
++    *errp
+)
+     ...>
+ }
+
+@@
+symbol errp;
+@@
+
+- *errp != NULL
++ *errp