Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

[Greg Kurz]

On Thu, 17 Sep 2020 22:55:18 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> qcow2_do_open correctly sets errp on each failure path. So, we can
> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> propagation. We should use ERRP_GUARD() (accordingly to comment in
> include/qapi/error.h) together with error_append() call which we add to
> avoid problems with error_fatal.
> 

The wording gives the impression that we add error_append() to avoid problems
with error_fatal which is certainly not true. Also it isn't _append() but
_prepend() :)

What about ?

"Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
 to avoid problems with the error_prepend() call if errp is &error_fatal."

With that fixed,

Reviewed-by: Greg Kurz <groug@kaod.org>

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2b6ec4b757..cd5f48d3fb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs)
>  static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>                                                     Error **errp)
>  {
> +    ERRP_GUARD();
>      BDRVQcow2State *s = bs->opaque;
>      int flags = s->flags;
>      QCryptoBlock *crypto = NULL;
>      QDict *options;
> -    Error *local_err = NULL;
>      int ret;
>  
>      /*
> @@ -2724,16 +2724,11 @@ static void coroutine_fn 
> qcow2_co_invalidate_cache(BlockDriverState *bs,
>  
>      flags &= ~BDRV_O_INACTIVE;
>      qemu_co_mutex_lock(&s->lock);
> -    ret = qcow2_do_open(bs, options, flags, &local_err);
> +    ret = qcow2_do_open(bs, options, flags, errp);
>      qemu_co_mutex_unlock(&s->lock);
>      qobject_unref(options);
> -    if (local_err) {
> -        error_propagate_prepend(errp, local_err,
> -                                "Could not reopen qcow2 layer: ");
> -        bs->drv = NULL;
> -        return;
> -    } else if (ret < 0) {
> -        error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
> +    if (ret < 0) {
> +        error_prepend(errp, "Could not reopen qcow2 layer: ");
>          bs->drv = NULL;
>          return;
>      }