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.

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

(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>
---
 monitor/qmp-cmds.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index c6faa3eaf0..2db3199a2e 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -126,9 +126,9 @@ void qmp_x_exit_preconfig(Error **errp)
 
 void qmp_cont(Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockBackend *blk;
     BlockJob *job;
-    Error *local_err = NULL;
 
     /* if there is a dump in background, we should wait until the dump
      * finished */
@@ -161,9 +161,8 @@ void qmp_cont(Error **errp)
      * If there are no inactive block nodes (e.g. because the VM was just
      * paused rather than completing a migration), bdrv_inactivate_all() simply
      * doesn't do anything. */
-    bdrv_invalidate_cache_all(&local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    bdrv_invalidate_cache_all(errp);
+    if (*errp) {
         return;
     }
 
-- 
2.21.0

On 10/11/19 11:05 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)
> 

> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   monitor/qmp-cmds.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index c6faa3eaf0..2db3199a2e 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -126,9 +126,9 @@ void qmp_x_exit_preconfig(Error **errp)
>   
>   void qmp_cont(Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       BlockBackend *blk;
>       BlockJob *job;
> -    Error *local_err = NULL;
>   
>       /* if there is a dump in background, we should wait until the dump
>        * finished */
> @@ -161,9 +161,8 @@ void qmp_cont(Error **errp)
>        * If there are no inactive block nodes (e.g. because the VM was just
>        * paused rather than completing a migration), bdrv_inactivate_all() simply
>        * doesn't do anything. */
> -    bdrv_invalidate_cache_all(&local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    bdrv_invalidate_cache_all(errp);
> +    if (*errp) {
>           return;
>       }
>   

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

[As elsewhere in this RFC series, my first pass on review is limited to 
just checking that what Coccinelle did is correct, rather than also 
checking whether Coccinelle missed things or whether I could reproduce 
the Coccinelle results]

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