Re: [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()

[Vladimir Sementsov-Ogievskiy]

20.11.2017 19:23, Kevin Wolf wrote:
> Am 20.11.2017 um 17:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 20.11.2017 17:50, Kevin Wolf wrote:
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > ---
> > >    include/block/snapshot.h |  3 ++-
> > >    block/snapshot.c         | 21 ++++++++++++++++-----
> > >    qemu-img.c               |  6 +++---
> > >    3 files changed, 21 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> > > index e5c0553115..aeb80405e8 100644
> > > --- a/include/block/snapshot.h
> > > +++ b/include/block/snapshot.h
> > > @@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
> > >    int bdrv_snapshot_create(BlockDriverState *bs,
> > >                             QEMUSnapshotInfo *sn_info);
> > >    int bdrv_snapshot_goto(BlockDriverState *bs,
> > > -                       const char *snapshot_id);
> > > +                       const char *snapshot_id,
> > > +                       Error **errp);
> > >    int bdrv_snapshot_delete(BlockDriverState *bs,
> > >                             const char *snapshot_id,
> > >                             const char *name,
> > > diff --git a/block/snapshot.c b/block/snapshot.c
> > > index be0743abac..9c941e638d 100644
> > > --- a/block/snapshot.c
> > > +++ b/block/snapshot.c
> > > @@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
> > >    }
> > >    int bdrv_snapshot_goto(BlockDriverState *bs,
> > > -                       const char *snapshot_id)
> > > +                       const char *snapshot_id,
> > > +                       Error **errp)
> > >    {
> > >        BlockDriver *drv = bs->drv;
> > >        int ret, open_ret;
> > >        int64_t len;
> > >        if (!drv) {
> > > +        error_setg(errp, "Block driver is closed");
> > >            return -ENOMEDIUM;
> > >        }
> > >        len = bdrv_getlength(bs);
> > >        if (len < 0) {
> > > +        error_setg_errno(errp, -len, "Cannot get block device size");
> > >            return len;
> > >        }
> > >        /* We should set all bits in all enabled dirty bitmaps, because dirty
> > > @@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > >        bdrv_set_dirty(bs, 0, len);
> > >        if (drv->bdrv_snapshot_goto) {
> > > -        return drv->bdrv_snapshot_goto(bs, snapshot_id);
> > > +        ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
> > > +        if (ret < 0) {
> > > +            error_setg_errno(errp, -ret, "Failed to load snapshot");
> > > +        }
> > > +        return ret;
> > >        }
> > >        if (bs->file) {
> > >            BlockDriverState *file;
> > >            QDict *options = qdict_clone_shallow(bs->options);
> > >            QDict *file_options;
> > > +        Error *local_err = NULL;
> > >            file = bs->file->bs;
> > >            /* Prevent it from getting deleted when detached from bs */
> > > @@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > >            bdrv_unref_child(bs, bs->file);
> > >            bs->file = NULL;
> > > -        ret = bdrv_snapshot_goto(file, snapshot_id);
> > > -        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> > > +        ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> > > +        open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
> > >            QDECREF(options);
> > >            if (open_ret < 0) {
> > >                bdrv_unref(file);
> > >                bs->drv = NULL;
> > > +            /* A bdrv_snapshot_goto() error takes precedence */
> > you return open_ret from bdrv_open and err msg from bdrv_snapshot_goto.
> > the may not correspond to each other.
> Hm, yes. Is this a problem, though?
>
> If it is, I guess we can change the return value below:
>
> > > +            error_propagate(errp, local_err);
> > >                return open_ret;
> return ret < 0 ? ret : open_ret;
>
> Would people prefer this?

looks good,

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> Kevin


--
Best regards,
Vladimir