Kevin Wolf <kw...@redhat.com> writes: > Am 28.05.2010 20:18, schrieb Miguel Di Ciurcio Filho: >> 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() and >> bdrv_has_snapshot() actually do what they must do and enforces better tests >> to >> avoid errors in the middle of do_savevm(). >> >> The functions were moved from savevm.c to block.c. It makes more sense to me. >> >> The bdrv_has_snapshot() is not beaultiful, but it does the job. I think >> having >> this function avaible in the BlockDriver would be the best option. >> >> 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> > > Markus, I think this implements mostly what we discussed the other day. > Not sure if you already have a patch for doing this - if so, maybe could > compare the patches and give it a review this way?
Glad I don't, because this one looks pretty good. > I seem to remember that we came to the conclusion that > bdrv_has_snapshot() isn't needed at all and should be dropped. Any user > should be using bdrv_can_snapshot() instead as this is what they really > want. Our reasoning adapted to the patched code goes like this. The remaining uses of bdrv_has_snapshot() are of the form: if (bdrv_has_snapshot(bs) some_snapshot_op() where some_snapshot_op() fails when there are no snapshots, and the code can recognize that failure as "sorry, no snapshots" (that's what it does before your patch, when bdrv_has_snapshot() is really no more than a necessary, but not sufficient condition for "has snapshots"). Therefore, we don't have to check for actual existence of snapshots with bdrv_has_snapshot(). bdrv_can_snapshot() suffices. In general, I consider if (predict_whether_foo_will_work) if (foo() < 0) // foo failed else // foo would have failed (we think) an anti-pattern. Just make foo() safe to try, and check its status: if (foo() < 0) // foo failed Except when foo() is too expensive to try, or can't be made safe. >> --- >> block.c | 47 ++++++++++++++++++++++++++++++++++++++++++----- >> block.h | 2 ++ >> savevm.c | 48 +++++++++++++++++++++++++----------------------- >> 3 files changed, 69 insertions(+), 28 deletions(-) >> >> diff --git a/block.c b/block.c >> index cd70730..7eddc15 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1720,15 +1720,52 @@ void bdrv_debug_event(BlockDriverState *bs, >> BlkDebugEvent event) >> /**************************************************************/ >> /* handling of snapshots */ >> >> -int bdrv_snapshot_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info) >> +int bdrv_can_snapshot(BlockDriverState *bs) >> { >> BlockDriver *drv = bs->drv; >> - if (!drv) >> + if (!drv) { >> + return -ENOMEDIUM; >> + } >> + >> + if (!drv->bdrv_snapshot_create || bdrv_is_removable(bs) || >> + bdrv_is_read_only(bs)) { >> + return -ENOTSUP; >> + } >> + >> + return 1; >> +} > > Returning either 1 or -errno is a strange interface. I'm not sure which Agree. > of 1/0 or 0/-errno is better in this case, but I'd suggest to take one > of these. Does any existing caller care for the error codes? If not, just go with 1/0. [...]