| Message ID | 1274091589-19991-3-git-send-email-morita.kazutaka@lab.ntt.co.jp |
|---|---|
| State | New |
| Headers | show |
Am 17.05.2010 12:19, schrieb MORITA Kazutaka: > When snapshot handlers are not defined in the format driver, it is > better to call the ones of the protocol driver. This enables us to > implement snapshot support in the protocol driver. > > We need to call bdrv_close() and bdrv_open() handlers of the format > driver before and after bdrv_snapshot_goto() call of the protocol. It is > because the contents of the block driver state may need to be changed > after loading vmstate. > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > --- > block.c | 61 +++++++++++++++++++++++++++++++++++++++++++------------------ > 1 files changed, 43 insertions(+), 18 deletions(-) > > diff --git a/block.c b/block.c > index f3bf3f2..c987e57 100644 > --- a/block.c > +++ b/block.c > @@ -1683,9 +1683,11 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, > BlockDriver *drv = bs->drv; > if (!drv) > return -ENOMEDIUM; > - if (!drv->bdrv_save_vmstate) > - return -ENOTSUP; > - return drv->bdrv_save_vmstate(bs, buf, pos, size); > + if (drv->bdrv_save_vmstate) > + return drv->bdrv_save_vmstate(bs, buf, pos, size); > + if (bs->file) > + return bdrv_save_vmstate(bs->file, buf, pos, size); > + return -ENOTSUP; > } > > int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, > @@ -1694,9 +1696,11 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, > BlockDriver *drv = bs->drv; > if (!drv) > return -ENOMEDIUM; > - if (!drv->bdrv_load_vmstate) > - return -ENOTSUP; > - return drv->bdrv_load_vmstate(bs, buf, pos, size); > + if (drv->bdrv_load_vmstate) > + return drv->bdrv_load_vmstate(bs, buf, pos, size); > + if (bs->file) > + return bdrv_load_vmstate(bs->file, buf, pos, size); > + return -ENOTSUP; > } > > void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event) > @@ -1720,20 +1724,37 @@ int bdrv_snapshot_create(BlockDriverState *bs, > BlockDriver *drv = bs->drv; > if (!drv) > return -ENOMEDIUM; > - if (!drv->bdrv_snapshot_create) > - return -ENOTSUP; > - return drv->bdrv_snapshot_create(bs, sn_info); > + if (drv->bdrv_snapshot_create) > + return drv->bdrv_snapshot_create(bs, sn_info); > + if (bs->file) > + return bdrv_snapshot_create(bs->file, sn_info); > + return -ENOTSUP; > } > > int bdrv_snapshot_goto(BlockDriverState *bs, > const char *snapshot_id) > { > BlockDriver *drv = bs->drv; > + int ret, open_ret; > + > if (!drv) > return -ENOMEDIUM; > - if (!drv->bdrv_snapshot_goto) > - return -ENOTSUP; > - return drv->bdrv_snapshot_goto(bs, snapshot_id); > + if (drv->bdrv_snapshot_goto) > + return drv->bdrv_snapshot_goto(bs, snapshot_id); > + > + if (bs->file) { > + drv->bdrv_close(bs); > + ret = bdrv_snapshot_goto(bs->file, snapshot_id); > + open_ret = drv->bdrv_open(bs, bs->open_flags); > + if (open_ret < 0) { > + bdrv_delete(bs); I think you mean bs->file here. Kevin > + bs->drv = NULL; > + return open_ret; > + } > + return ret; > + } > + > + return -ENOTSUP; > } > > int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
At Mon, 17 May 2010 13:08:08 +0200, Kevin Wolf wrote: > > Am 17.05.2010 12:19, schrieb MORITA Kazutaka: > > > > int bdrv_snapshot_goto(BlockDriverState *bs, > > const char *snapshot_id) > > { > > BlockDriver *drv = bs->drv; > > + int ret, open_ret; > > + > > if (!drv) > > return -ENOMEDIUM; > > - if (!drv->bdrv_snapshot_goto) > > - return -ENOTSUP; > > - return drv->bdrv_snapshot_goto(bs, snapshot_id); > > + if (drv->bdrv_snapshot_goto) > > + return drv->bdrv_snapshot_goto(bs, snapshot_id); > > + > > + if (bs->file) { > > + drv->bdrv_close(bs); > > + ret = bdrv_snapshot_goto(bs->file, snapshot_id); > > + open_ret = drv->bdrv_open(bs, bs->open_flags); > > + if (open_ret < 0) { > > + bdrv_delete(bs); > > I think you mean bs->file here. > > Kevin This is an error of re-opening the format driver, so what we should delete here is not bs->file but bs, isn't it? If we failed to open bs here, the drive doesn't seem to work anymore. Regards, Kazutaka > > + bs->drv = NULL; > > + return open_ret; > > + } > > + return ret; > > + } > > + > > + return -ENOTSUP; > > }
Am 17.05.2010 14:19, schrieb MORITA Kazutaka: > At Mon, 17 May 2010 13:08:08 +0200, > Kevin Wolf wrote: >> >> Am 17.05.2010 12:19, schrieb MORITA Kazutaka: >>> >>> int bdrv_snapshot_goto(BlockDriverState *bs, >>> const char *snapshot_id) >>> { >>> BlockDriver *drv = bs->drv; >>> + int ret, open_ret; >>> + >>> if (!drv) >>> return -ENOMEDIUM; >>> - if (!drv->bdrv_snapshot_goto) >>> - return -ENOTSUP; >>> - return drv->bdrv_snapshot_goto(bs, snapshot_id); >>> + if (drv->bdrv_snapshot_goto) >>> + return drv->bdrv_snapshot_goto(bs, snapshot_id); >>> + >>> + if (bs->file) { >>> + drv->bdrv_close(bs); >>> + ret = bdrv_snapshot_goto(bs->file, snapshot_id); >>> + open_ret = drv->bdrv_open(bs, bs->open_flags); >>> + if (open_ret < 0) { >>> + bdrv_delete(bs); >> >> I think you mean bs->file here. >> >> Kevin > > This is an error of re-opening the format driver, so what we should > delete here is not bs->file but bs, isn't it? If we failed to open bs > here, the drive doesn't seem to work anymore. But bdrv_delete means basically free it. This is almost guaranteed to lead to crashes because that BlockDriverState is still in use in other places. One additional case of use after free is in the very next line: >>> + bs->drv = NULL; You can't do that when bs is freed, obviously. But I think just setting bs->drv to NULL without bdrv_deleting it before is the better way. It will fail any requests (with -ENOMEDIUM), but can't produce crashes. This is also what bdrv_commit does in such situations. In this state, we don't access the underlying file any more, so we could delete bs->file - this is why I thought you actually meant to do that. Kevin
On Mon, May 17, 2010 at 9:20 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 17.05.2010 14:19, schrieb MORITA Kazutaka: >> At Mon, 17 May 2010 13:08:08 +0200, >> Kevin Wolf wrote: >>> >>> Am 17.05.2010 12:19, schrieb MORITA Kazutaka: >>>> >>>> int bdrv_snapshot_goto(BlockDriverState *bs, >>>> const char *snapshot_id) >>>> { >>>> BlockDriver *drv = bs->drv; >>>> + int ret, open_ret; >>>> + >>>> if (!drv) >>>> return -ENOMEDIUM; >>>> - if (!drv->bdrv_snapshot_goto) >>>> - return -ENOTSUP; >>>> - return drv->bdrv_snapshot_goto(bs, snapshot_id); >>>> + if (drv->bdrv_snapshot_goto) >>>> + return drv->bdrv_snapshot_goto(bs, snapshot_id); >>>> + >>>> + if (bs->file) { >>>> + drv->bdrv_close(bs); >>>> + ret = bdrv_snapshot_goto(bs->file, snapshot_id); >>>> + open_ret = drv->bdrv_open(bs, bs->open_flags); >>>> + if (open_ret < 0) { >>>> + bdrv_delete(bs); >>> >>> I think you mean bs->file here. >>> >>> Kevin >> >> This is an error of re-opening the format driver, so what we should >> delete here is not bs->file but bs, isn't it? If we failed to open bs >> here, the drive doesn't seem to work anymore. > > But bdrv_delete means basically free it. This is almost guaranteed to > lead to crashes because that BlockDriverState is still in use in other > places. > > One additional case of use after free is in the very next line: > >>>> + bs->drv = NULL; > > You can't do that when bs is freed, obviously. But I think just setting > bs->drv to NULL without bdrv_deleting it before is the better way. It > will fail any requests (with -ENOMEDIUM), but can't produce crashes. > This is also what bdrv_commit does in such situations. > > In this state, we don't access the underlying file any more, so we could > delete bs->file - this is why I thought you actually meant to do that. > I'm sorry for the confusion. I understand what we should do here. I'll fix it for the next post. Thanks, Kazutaka
diff --git a/block.c b/block.c index f3bf3f2..c987e57 100644 --- a/block.c +++ b/block.c @@ -1683,9 +1683,11 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_save_vmstate) - return -ENOTSUP; - return drv->bdrv_save_vmstate(bs, buf, pos, size); + if (drv->bdrv_save_vmstate) + return drv->bdrv_save_vmstate(bs, buf, pos, size); + if (bs->file) + return bdrv_save_vmstate(bs->file, buf, pos, size); + return -ENOTSUP; } int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, @@ -1694,9 +1696,11 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_load_vmstate) - return -ENOTSUP; - return drv->bdrv_load_vmstate(bs, buf, pos, size); + if (drv->bdrv_load_vmstate) + return drv->bdrv_load_vmstate(bs, buf, pos, size); + if (bs->file) + return bdrv_load_vmstate(bs->file, buf, pos, size); + return -ENOTSUP; } void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event) @@ -1720,20 +1724,37 @@ int bdrv_snapshot_create(BlockDriverState *bs, BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_snapshot_create) - return -ENOTSUP; - return drv->bdrv_snapshot_create(bs, sn_info); + if (drv->bdrv_snapshot_create) + return drv->bdrv_snapshot_create(bs, sn_info); + if (bs->file) + return bdrv_snapshot_create(bs->file, sn_info); + return -ENOTSUP; } int bdrv_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) { BlockDriver *drv = bs->drv; + int ret, open_ret; + if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_snapshot_goto) - return -ENOTSUP; - return drv->bdrv_snapshot_goto(bs, snapshot_id); + if (drv->bdrv_snapshot_goto) + return drv->bdrv_snapshot_goto(bs, snapshot_id); + + if (bs->file) { + drv->bdrv_close(bs); + ret = bdrv_snapshot_goto(bs->file, snapshot_id); + open_ret = drv->bdrv_open(bs, bs->open_flags); + if (open_ret < 0) { + bdrv_delete(bs); + bs->drv = NULL; + return open_ret; + } + return ret; + } + + return -ENOTSUP; } int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) @@ -1741,9 +1762,11 @@ int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_snapshot_delete) - return -ENOTSUP; - return drv->bdrv_snapshot_delete(bs, snapshot_id); + if (drv->bdrv_snapshot_delete) + return drv->bdrv_snapshot_delete(bs, snapshot_id); + if (bs->file) + return bdrv_snapshot_delete(bs->file, snapshot_id); + return -ENOTSUP; } int bdrv_snapshot_list(BlockDriverState *bs, @@ -1752,9 +1775,11 @@ int bdrv_snapshot_list(BlockDriverState *bs, BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_snapshot_list) - return -ENOTSUP; - return drv->bdrv_snapshot_list(bs, psn_info); + if (drv->bdrv_snapshot_list) + return drv->bdrv_snapshot_list(bs, psn_info); + if (bs->file) + return bdrv_snapshot_list(bs->file, psn_info); + return -ENOTSUP; } #define NB_SUFFIXES 4
When snapshot handlers are not defined in the format driver, it is better to call the ones of the protocol driver. This enables us to implement snapshot support in the protocol driver. We need to call bdrv_close() and bdrv_open() handlers of the format driver before and after bdrv_snapshot_goto() call of the protocol. It is because the contents of the block driver state may need to be changed after loading vmstate. Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> --- block.c | 61 +++++++++++++++++++++++++++++++++++++++++++------------------ 1 files changed, 43 insertions(+), 18 deletions(-)