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

[Philippe Mathieu-Daudé]

On 10/12/20 12:07 PM, Max Reitz wrote:
> 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:

Indeed, thanks for catching that.

[...]
> >   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);

This one is not needed.

> >           }
> >           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;
> >           }
> >       }