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

[Kevin Wolf]

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?

Kevin