[RFC,v2,6/9] scripts: add coccinelle script to use auto propagated errp

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 23, 2019, 4:12 p.m. UTC

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

Comments

Eric Blake Sept. 23, 2019, 8:05 p.m. UTC | #1

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> ---
>  scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++++++++++++++
>  1 file changed, 82 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..1a3f006f0b
> --- /dev/null
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -0,0 +1,82 @@
> +@@
> +identifier fn;
> +identifier local_err;
> +@@
> +
> + fn(..., Error **errp)
> + {
> ++    ERRP_FUNCTION_BEGIN();
> + }

This doesn't catch functions where Error **errp is not the last
parameter.  Some examples (some of which may need independent tweaking
in their own right for being inconsistent, although we DO want errp to
appear before any 'format, ...' arguments):

block/ssh.c:sftp_error_setg(Error **errp, BDRVSSHState *s, const char
*fs, ...)
exec.c:static void ram_block_add(RAMBlock *new_block, Error **errp, bool
shared)

Does running this Coccinelle script 2 times in a row add a second
ERRP_FUNCTION_BEGIN() line?  We want it to be idempotent (no changes on
a second run).  (Admittedly, I did not actually test that yet).  Also, I
don't know if this can be tweaked to avoid adding the line to a function
with an empty body, maybe:

 fn(..., Error **errp, ...)
 {
+    ERRP_FUNCTION_BEGIN();
     ...
 }

to only add it to a function that already has a body (thanks to the ...)
- but I'm fuzzy enough on Coccinelle that I may be saying something
totally wrong.

> +
> +@rule1@
> +identifier fn;
> +identifier local_err;
> +@@
> +
> + fn(..., Error **errp)
> + {
> +     <...
> +-    Error *local_err = NULL;
> +     ...>
> + }
> +
> +@@
> +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(errp, local_err);
> +)
> +     ...>
> + }
> +
> +@@
> +identifier rule1.fn;
> +identifier rule1.local_err;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +(
> +-    &local_err
> ++    errp
> +|
> +-    local_err
> ++    *errp
> +)
> +     ...>
> + }
> 

Overall, the script makes sense in my reading (but no idea if it
actually catches everything we want, or if it missed something).  At any
rate, once patch 7 is split into more manageable chunks, we can
definitely spot-check results to make sure they all look reasonable.

Eric Blake Sept. 23, 2019, 9:29 p.m. UTC | #2

On 9/23/19 3:05 PM, Eric Blake wrote:

> Does running this Coccinelle script 2 times in a row add a second
> ERRP_FUNCTION_BEGIN() line?  We want it to be idempotent (no changes on
> a second run).  (Admittedly, I did not actually test that yet).  Also, I
> don't know if this can be tweaked to avoid adding the line to a function
> with an empty body, maybe:
> 
>  fn(..., Error **errp, ...)
>  {
> +    ERRP_FUNCTION_BEGIN();
>      ...
>  }

Also untested:

 fn(..., Error **errp, ...)
 {
(
|
     ERRP_FUNCTION_BEGIN();
     ...
|
+    ERRP_FUNCTION_BEGIN()
     ...
)
 }


> Overall, the script makes sense in my reading (but no idea if it
> actually catches everything we want, or if it missed something).

Having spot-checked 7, it definitely misses cases where it was supposed
to add ERRP_FUNCTION_BEGIN().

Vladimir Sementsov-Ogievskiy Sept. 24, 2019, 10:35 a.m. UTC | #3

24.09.2019 0:29, Eric Blake wrote:
> On 9/23/19 3:05 PM, Eric Blake wrote:
> 
>> Does running this Coccinelle script 2 times in a row add a second
>> ERRP_FUNCTION_BEGIN() line?  We want it to be idempotent (no changes on
>> a second run).  (Admittedly, I did not actually test that yet).  Also, I
>> don't know if this can be tweaked to avoid adding the line to a function
>> with an empty body, maybe:
>>
>>   fn(..., Error **errp, ...)
>>   {
>> +    ERRP_FUNCTION_BEGIN();
>>       ...
>>   }

No, we need exactly this to match not only empty functions. But with ... it matches
empty functions as well.

> 
> Also untested:
> 
>   fn(..., Error **errp, ...)
>   {
> (
> |
>       ERRP_FUNCTION_BEGIN();
>       ...
> |
> +    ERRP_FUNCTION_BEGIN()
>       ...
> )
>   }

Seems, that doesn't work..

It says:
12: no available token to attach to

where 12 is line "+    ERRP_FUNCTION_BEGIN()"


So, I tend to just add chunk to remove duplicated invocation :)

> 
> 
>> Overall, the script makes sense in my reading (but no idea if it
>> actually catches everything we want, or if it missed something).
> 
> Having spot-checked 7, it definitely misses cases where it was supposed
> to add ERRP_FUNCTION_BEGIN().
>

Patch

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 0000000000..1a3f006f0b
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,82 @@ 
+@@
+identifier fn;
+identifier local_err;
+@@
+
+ fn(..., Error **errp)
+ {
++    ERRP_FUNCTION_BEGIN();
+ }
+
+@rule1@
+identifier fn;
+identifier local_err;
+@@
+
+ fn(..., Error **errp)
+ {
+     <...
+-    Error *local_err = NULL;
+     ...>
+ }
+
+@@
+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(errp, local_err);
+)
+     ...>
+ }
+
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+     <...
+(
+-    &local_err
++    errp
+|
+-    local_err
++    *errp
+)
+     ...>
+ }