Re: [PATCH 1/3] block: push error reporting into bdrv_all_*_snapshot functions

[Max Reitz]

On 08.10.20 19:48, Philippe Mathieu-Daudé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> The bdrv_all_*_snapshot functions return a BlockDriverState pointer
> for the invalid backend, which the callers then use to report an
> error message. In some cases multiple callers are reporting the
> same error message, but with slightly different text. In the future
> there will be more error scenarios for some of these methods, which
> will benefit from fine grained error message reporting. So it is
> helpful to push error reporting down a level.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/block/snapshot.h       | 14 +++----
>  block/monitor/block-hmp-cmds.c |  7 ++--
>  block/snapshot.c               | 77 +++++++++++++++++-----------------
>  migration/savevm.c             | 37 +++++-----------
>  monitor/hmp-cmds.c             |  7 +---
>  replay/replay-debugging.c      |  4 +-
>  tests/qemu-iotests/267.out     | 10 ++---
>  7 files changed, 67 insertions(+), 89 deletions(-)

Looks good overall to me, but for some reason this patch pulls in the
@ok and @ret variables from the top scope of all concerned functions
into the inner scopes of the BDS loops, and drops their initialization.
 That’s wrong, because we only call the respective snapshotting
functions on some BDSs, so the return value stays uninitialized for all
other BDSs:

> diff --git a/block/snapshot.c b/block/snapshot.c
> index a2bf3a54eb..50e35bb9fa 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -462,14 +462,14 @@ static bool 
> bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
>   * These functions will properly handle dataplane (take aio_context_acquire
>   * when appropriate for appropriate block drivers) */
>  
> -bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
> +bool bdrv_all_can_snapshot(Error **errp)
>  {
> -    bool ok = true;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        bool ok;

So I think @ok must be initialized.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs)) {
> @@ -477,26 +477,25 @@ bool bdrv_all_can_snapshot(BlockDriverState 
> **first_bad_bs)
>          }
>          aio_context_release(ctx);
>          if (!ok) {
> +            error_setg(errp, "Device '%s' is writable but does not support "
> +                       "snapshots", bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return false;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return ok;
> +    return true;
>  }
>  
> -int bdrv_all_delete_snapshot(const char *name, BlockDriverState 
> **first_bad_bs,
> -                             Error **errp)
> +int bdrv_all_delete_snapshot(const char *name, Error **errp)
>  {
> -    int ret = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

Same here, @ret must be initialized.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs) &&
> @@ -507,26 +506,25 @@ int bdrv_all_delete_snapshot(const char *name, 
> BlockDriverState **first_bad_bs,
>          }
>          aio_context_release(ctx);
>          if (ret < 0) {
> +            error_prepend(errp, "Could not delete snapshot '%s' on '%s': ",
> +                          name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return ret;
> +    return 0;
>  }
>  
>  
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> -                           Error **errp)
> +int bdrv_all_goto_snapshot(const char *name, Error **errp)
>  {
> -    int ret = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

And again.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs)) {
> @@ -534,75 +532,75 @@ int bdrv_all_goto_snapshot(const char *name, 
> BlockDriverState **first_bad_bs,
>          }
>          aio_context_release(ctx);
>          if (ret < 0) {
> +            error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
> +                          name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return ret;
> +    return 0;
>  }
>  
> -int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +int bdrv_all_find_snapshot(const char *name, Error **errp)
>  {
>      QEMUSnapshotInfo sn;
> -    int err = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

Again.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs)) {
> -            err = bdrv_snapshot_find(bs, &sn, name);
> +            ret = bdrv_snapshot_find(bs, &sn, name);
>          }
>          aio_context_release(ctx);
> -        if (err < 0) {
> +        if (ret < 0) {
> +            error_setg(errp, "Could not find snapshot '%s' on '%s'",
> +                       name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return err;
> +    return 0;
>  }
>  
>  int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>                               BlockDriverState *vm_state_bs,
>                               uint64_t vm_state_size,
> -                             BlockDriverState **first_bad_bs)
> +                             Error **errp)
>  {
> -    int err = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

And one final time.

Max

>          aio_context_acquire(ctx);
>          if (bs == vm_state_bs) {
>              sn->vm_state_size = vm_state_size;
> -            err = bdrv_snapshot_create(bs, sn);
> +            ret = bdrv_snapshot_create(bs, sn);
>          } else if (bdrv_all_snapshots_includes_bs(bs)) {
>              sn->vm_state_size = 0;
> -            err = bdrv_snapshot_create(bs, sn);
> +            ret = bdrv_snapshot_create(bs, sn);
>          }
>          aio_context_release(ctx);
> -        if (err < 0) {
> +        if (ret < 0) {
> +            error_setg(errp, "Could not create snapshot '%s' on '%s'",
> +                       sn->name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }