Re: [PATCH v6 03/11] scripts: add coccinelle script to use auto propagated errp

[Eric Blake <eblake@redhat.com>]

On 1/10/20 1:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Rather light on the commit message. If nothing else, a comment about 
typical command-line usage would be helpful (yes, it's in the patch 
body, but sometimes I just refer to git log).

> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 532b9afb9e..dcfb77e107 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -141,6 +141,9 @@
>    *         ...
>    *     }
>    *
> + * For mass conversion use script
> + *   scripts/coccinelle/auto-propagated-errp.cocci
> + *
>    *
>    * Receive and accumulate multiple errors (first one wins):
>    *     Error *err = NULL, *local_err = NULL;
> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
> new file mode 100644
> index 0000000000..6c72a5049f
> --- /dev/null
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -0,0 +1,139 @@
> +// Use ERRP_AUTO_PROPAGATE (see 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 \
> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
> +
> +@@
> +// Add invocation to errp-functions where necessary
> +// We should skip functions with "Error *const *errp"
> +// parameter, but how to do it with coccinelle?
> +// I don't know, so, I skip them by function name regex.
> +// It's safe: if we not skip some functions with

s/not/did not/

> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
> +// will fail to compile, because of const violation.
> +identifier fn !~ "error_append_.*_hint";
> +identifier local_err, errp;
> +@@
> +
> + fn(..., Error **errp, ...)
> + {
> ++   ERRP_AUTO_PROPAGATE();
> +    <+...
> +        when != ERRP_AUTO_PROPAGATE();
> +(
> +    error_append_hint(errp, ...);
> +|
> +    error_prepend(errp, ...);
> +|
> +    Error *local_err = NULL;
> +)
> +    ...+>
> + }
> +

Looks like it should catch all functions that require adding the macro.

> +@rule1@
> +// We do not inherit from previous rule, as we want to match
> +// also functions, which already had ERRP_AUTO_PROPAGATE
> +// invocation.

Grammar suggestion:

// We want to patch error propagation in functions regardless of
// whether the function already uses ERRP_AUTO_PROPAGATE, hence
// this one does not inherit from the first rule.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org