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

[Eric Blake]

On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  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, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org