At Fri, 4 Jun 2010 16:35:59 -0300, Miguel Di Ciurcio Filho wrote: > > Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized. > > First issue: Their names implies different porpouses, but they do the same > thing > and have exactly the same code. Maybe copied and pasted and forgotten? > bdrv_has_snapshot() is called in various places for actually checking if there > is snapshots or not. > > Second issue: the way bdrv_can_snapshot() verifies if a block driver supports > or > not snapshots does not catch all cases. E.g.: a raw image. > > So when do_savevm() is called, first thing it does is to set a global > BlockDriverState to save the VM memory state calling get_bs_snapshots(). > > static BlockDriverState *get_bs_snapshots(void) > { > BlockDriverState *bs; > DriveInfo *dinfo; > > if (bs_snapshots) > return bs_snapshots; > QTAILQ_FOREACH(dinfo, &drives, next) { > bs = dinfo->bdrv; > if (bdrv_can_snapshot(bs)) > goto ok; > } > return NULL; > ok: > bs_snapshots = bs; > return bs; > } > > bdrv_can_snapshot() may return a BlockDriverState that does not support > snapshots and do_savevm() goes on. > > Later on in do_savevm(), we find: > > QTAILQ_FOREACH(dinfo, &drives, next) { > bs1 = dinfo->bdrv; > if (bdrv_has_snapshot(bs1)) { > /* Write VM state size only to the image that contains the state > */ > sn->vm_state_size = (bs == bs1 ? vm_state_size : 0); > ret = bdrv_snapshot_create(bs1, sn); > if (ret < 0) { > monitor_printf(mon, "Error while creating snapshot on '%s'\n", > bdrv_get_device_name(bs1)); > } > } > } > > bdrv_has_snapshot(bs1) is not checking if the device does support or has > snapshots as explained above. Only in bdrv_snapshot_create() the device is > actually checked for snapshot support. > > So, in cases where the first device supports snapshots, and the second does > not, > the snapshot on the first will happen anyways. I believe this is not a good > behavior. It should be an all or nothing process. > > This patch addresses these issues by making bdrv_can_snapshot() actually do > what it must do and enforces better tests to avoid errors in the middle of > do_savevm(). bdrv_has_snapshot() is removed and replaced by > bdrv_can_snapshot() > where appropriate. > > bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more sense > to me. > > The loadvm_state() function was updated too to enforce that when loading a VM > at > least all writable devices must support snapshots too. > > Signed-off-by: Miguel Di Ciurcio Filho <miguel.fi...@gmail.com> > --- > block.c | 11 +++++++++++ > block.h | 1 + > savevm.c | 58 ++++++++++++++++++++++++++++++++++++---------------------- > 3 files changed, 48 insertions(+), 22 deletions(-) > > diff --git a/block.c b/block.c > index cd70730..ace3cdb 100644 > --- a/block.c > +++ b/block.c > @@ -1720,6 +1720,17 @@ void bdrv_debug_event(BlockDriverState *bs, > BlkDebugEvent event) > /**************************************************************/ > /* handling of snapshots */ > > +int bdrv_can_snapshot(BlockDriverState *bs) > +{ > + BlockDriver *drv = bs->drv; > + if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) || > + bdrv_is_read_only(bs)) { > + return 0; > + } > + > + return 1; > +} > +
The underlying protocol could support snapshots, so I think we should check against bs->file too. --- a/block.c +++ b/block.c @@ -1671,6 +1671,9 @@ int bdrv_can_snapshot(BlockDriverState *bs) BlockDriver *drv = bs->drv; if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) || bdrv_is_read_only(bs)) { + if (bs->file) { + return bdrv_can_snapshot(bs->file); + } return 0; } Regards, Kazutaka