Re: [RFC v5 027/126] misc: introduce ERRP_AUTO_PROPAGATE

[Eric Blake <eblake@redhat.com>]

On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> If we want to add some info to errp (by error_prepend() or
> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
> Otherwise, this info will not be added when errp == &fatal_err
> (the program will exit prior to the error_append_hint() or
> error_prepend() call).  Fix such cases.
> 
> If we want to check error after errp-function call, we need to
> introduce local_err and than propagate it to errp. Instead, use
> ERRP_AUTO_PROPAGATE macro, benefits are:
> 1. No need of explicit error_propagate call
> 2. No need of explicit local_err variable: use errp directly
> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
>     &error_fatel, this means that we don't break error_abort
>     (we'll abort on error_set, not on error_propagate)
> 
> This commit (together with its neighbors) was generated by
> 
> for f in $(git grep -l errp \*.[ch]); do \
>      spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>      --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
> done;
> 
> then fix a bit of compilation problems: coccinelle for some reason
> leaves several
> f() {
>      ...
>      goto out;
>      ...
>      out:
> }
> patterns, with "out:" at function end.

Was that still happening even after your tweaks to the .cocci script? 
But manual touch-up after cocci is not unheard of, so it is not a 
showstopper to the series.  Still, it might be nicer if this disclaimer 
only appears on the patches within the series where it actually matters, 
rather than on every message in the series even when no tweaks were 
needed (as this patch is an example where the touchup was not needed).

> 
> then
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 

If we don't check the python script into git, then changing this to a 
URL to one of the threads where you posted the script in an earlier 
version of the patch is also acceptable.

> (auto-msg was a file with this commit message)
> 
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   hw/misc/ivshmem.c | 37 ++++++++++++++++---------------------
>   hw/misc/tmp105.c  |  7 +++----
>   hw/misc/tmp421.c  |  7 +++----
>   3 files changed, 22 insertions(+), 29 deletions(-)
> 

> @@ -864,6 +858,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>   
>   static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       IVShmemState *s = IVSHMEM_COMMON(dev);
>       Error *err = NULL;

Umm, so why did Coccinelle not remove this line, or retouch lower down at:

     if (!ivshmem_is_master(s)) {
         error_setg(&s->migration_blocker,
                    "Migration is disabled when using feature 'peer 
mode' in devi
ce 'ivshmem'");
         migrate_add_blocker(s->migration_blocker, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             error_free(s->migration_blocker);
             return;
         }
     }


But the conversions that Coccinelle made look correct.

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