| Message ID | 1275070726-18531-1-git-send-email-miguel.filho@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.filho@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? 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. > --- > 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 of 1/0 or 0/-errno is better in this case, but I'd suggest to take one of these. > +int bdrv_has_snapshot(BlockDriverState *bs) > +{ > + int ret; > + QEMUSnapshotInfo *sn_tab; > + BlockDriver *drv = bs->drv; > + if (!drv) { > return -ENOMEDIUM; > - if (!drv->bdrv_snapshot_create) > + } > + > + if (!drv->bdrv_snapshot_list) { > return -ENOTSUP; > - return drv->bdrv_snapshot_create(bs, sn_info); > + } > + > + ret = drv->bdrv_snapshot_list(bs, &sn_tab); > + > + if (sn_tab) { > + qemu_free(sn_tab); > + } > + > + return ret; > +} > + > +int bdrv_snapshot_create(BlockDriverState *bs, > + QEMUSnapshotInfo *sn_info) > +{ > + BlockDriver *drv = bs->drv; > + if (bdrv_can_snapshot(bs) > 0) { > + return drv->bdrv_snapshot_create(bs, sn_info); > + } > + > + return -1; > } > > int bdrv_snapshot_goto(BlockDriverState *bs, > diff --git a/block.h b/block.h > index 24efeb6..c536f1c 100644 > --- a/block.h > +++ b/block.h > @@ -173,6 +173,8 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); > const char *bdrv_get_encrypted_filename(BlockDriverState *bs); > void bdrv_get_backing_filename(BlockDriverState *bs, > char *filename, int filename_size); > +int bdrv_can_snapshot(BlockDriverState *bs); > +int bdrv_has_snapshot(BlockDriverState *bs); > int bdrv_snapshot_create(BlockDriverState *bs, > QEMUSnapshotInfo *sn_info); > int bdrv_snapshot_goto(BlockDriverState *bs, > diff --git a/savevm.c b/savevm.c > index dc20390..9bc232f 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1574,22 +1574,6 @@ out: > return ret; > } > > -/* device can contain snapshots */ > -static int bdrv_can_snapshot(BlockDriverState *bs) > -{ > - return (bs && > - !bdrv_is_removable(bs) && > - !bdrv_is_read_only(bs)); > -} > - > -/* device must be snapshots in order to have a reliable snapshot */ > -static int bdrv_has_snapshot(BlockDriverState *bs) > -{ > - return (bs && > - !bdrv_is_removable(bs) && > - !bdrv_is_read_only(bs)); > -} > - > static BlockDriverState *get_bs_snapshots(void) > { > BlockDriverState *bs; > @@ -1599,7 +1583,7 @@ static BlockDriverState *get_bs_snapshots(void) > return bs_snapshots; > QTAILQ_FOREACH(dinfo, &drives, next) { > bs = dinfo->bdrv; > - if (bdrv_can_snapshot(bs)) > + if (bdrv_can_snapshot(bs) > 0) > goto ok; > } > return NULL; > @@ -1642,7 +1626,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > > QTAILQ_FOREACH(dinfo, &drives, next) { > bs = dinfo->bdrv; > - if (bdrv_can_snapshot(bs) && > + if ((bdrv_can_snapshot(bs) > 0) && > bdrv_snapshot_find(bs, snapshot, name) >= 0) > { > ret = bdrv_snapshot_delete(bs, name); > @@ -1674,12 +1658,30 @@ void do_savevm(Monitor *mon, const QDict *qdict) > #endif > const char *name = qdict_get_try_str(qdict, "name"); > > + /* Verify if there is have a device that doesn't support snapshots and is writable*/ > + ret = 0; > + QTAILQ_FOREACH(dinfo, &drives, next) { > + bs = dinfo->bdrv; > + > + if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) { > + continue; > + } > + > + if ((ret = bdrv_can_snapshot(bs)) < 0) { This may overwrite the error of the previous loop iteration with a success return value. It's probably not what you want. Other than that it looks good to me. Kevin
Kevin Wolf <kwolf@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.filho@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. [...]
On Sat, May 29, 2010 at 3:06 AM, Markus Armbruster <armbru@redhat.com> wrote: > >> 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. I had this feeling too, but I was conservative and kept bdrv_has_snapshot(). I will remove it and replace by bdrv_can_snapshot() where appropriate. >>> +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. When bdrv_can_snapshot() is called the specific reason for not supporting snapshots does not matter. So, 1/0 is better. I will fix it and resend. Regards, Miguel
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; +} + +int bdrv_has_snapshot(BlockDriverState *bs) +{ + int ret; + QEMUSnapshotInfo *sn_tab; + BlockDriver *drv = bs->drv; + if (!drv) { return -ENOMEDIUM; - if (!drv->bdrv_snapshot_create) + } + + if (!drv->bdrv_snapshot_list) { return -ENOTSUP; - return drv->bdrv_snapshot_create(bs, sn_info); + } + + ret = drv->bdrv_snapshot_list(bs, &sn_tab); + + if (sn_tab) { + qemu_free(sn_tab); + } + + return ret; +} + +int bdrv_snapshot_create(BlockDriverState *bs, + QEMUSnapshotInfo *sn_info) +{ + BlockDriver *drv = bs->drv; + if (bdrv_can_snapshot(bs) > 0) { + return drv->bdrv_snapshot_create(bs, sn_info); + } + + return -1; } int bdrv_snapshot_goto(BlockDriverState *bs, diff --git a/block.h b/block.h index 24efeb6..c536f1c 100644 --- a/block.h +++ b/block.h @@ -173,6 +173,8 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); const char *bdrv_get_encrypted_filename(BlockDriverState *bs); void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); +int bdrv_can_snapshot(BlockDriverState *bs); +int bdrv_has_snapshot(BlockDriverState *bs); int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int bdrv_snapshot_goto(BlockDriverState *bs, diff --git a/savevm.c b/savevm.c index dc20390..9bc232f 100644 --- a/savevm.c +++ b/savevm.c @@ -1574,22 +1574,6 @@ out: return ret; } -/* device can contain snapshots */ -static int bdrv_can_snapshot(BlockDriverState *bs) -{ - return (bs && - !bdrv_is_removable(bs) && - !bdrv_is_read_only(bs)); -} - -/* device must be snapshots in order to have a reliable snapshot */ -static int bdrv_has_snapshot(BlockDriverState *bs) -{ - return (bs && - !bdrv_is_removable(bs) && - !bdrv_is_read_only(bs)); -} - static BlockDriverState *get_bs_snapshots(void) { BlockDriverState *bs; @@ -1599,7 +1583,7 @@ static BlockDriverState *get_bs_snapshots(void) return bs_snapshots; QTAILQ_FOREACH(dinfo, &drives, next) { bs = dinfo->bdrv; - if (bdrv_can_snapshot(bs)) + if (bdrv_can_snapshot(bs) > 0) goto ok; } return NULL; @@ -1642,7 +1626,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) QTAILQ_FOREACH(dinfo, &drives, next) { bs = dinfo->bdrv; - if (bdrv_can_snapshot(bs) && + if ((bdrv_can_snapshot(bs) > 0) && bdrv_snapshot_find(bs, snapshot, name) >= 0) { ret = bdrv_snapshot_delete(bs, name); @@ -1674,12 +1658,30 @@ void do_savevm(Monitor *mon, const QDict *qdict) #endif const char *name = qdict_get_try_str(qdict, "name"); + /* Verify if there is have a device that doesn't support snapshots and is writable*/ + ret = 0; + QTAILQ_FOREACH(dinfo, &drives, next) { + bs = dinfo->bdrv; + + if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) { + continue; + } + + if ((ret = bdrv_can_snapshot(bs)) < 0) { + monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n", + bdrv_get_device_name(bs)); + } + } + + if (ret < 0) { + goto the_end; + } + bs = get_bs_snapshots(); if (!bs) { monitor_printf(mon, "No block device can accept snapshots\n"); return; } - /* ??? Should this occur after vm_stop? */ qemu_aio_flush(); @@ -1732,7 +1734,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) QTAILQ_FOREACH(dinfo, &drives, next) { bs1 = dinfo->bdrv; - if (bdrv_has_snapshot(bs1)) { + if (bdrv_can_snapshot(bs1) > 0) { /* 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); @@ -1767,7 +1769,7 @@ int load_vmstate(const char *name) QTAILQ_FOREACH(dinfo, &drives, next) { bs1 = dinfo->bdrv; - if (bdrv_has_snapshot(bs1)) { + if (bdrv_has_snapshot(bs1) > 0) { ret = bdrv_snapshot_goto(bs1, name); if (ret < 0) { switch(ret) { @@ -1829,7 +1831,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) QTAILQ_FOREACH(dinfo, &drives, next) { bs1 = dinfo->bdrv; - if (bdrv_has_snapshot(bs1)) { + if (bdrv_has_snapshot(bs1) > 0) { ret = bdrv_snapshot_delete(bs1, name); if (ret < 0) { if (ret == -ENOTSUP) @@ -1860,7 +1862,7 @@ void do_info_snapshots(Monitor *mon) monitor_printf(mon, "Snapshot devices:"); QTAILQ_FOREACH(dinfo, &drives, next) { bs1 = dinfo->bdrv; - if (bdrv_has_snapshot(bs1)) { + if (bdrv_can_snapshot(bs1) > 0) { if (bs == bs1) monitor_printf(mon, " %s", bdrv_get_device_name(bs1)); }
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.filho@gmail.com> --- block.c | 47 ++++++++++++++++++++++++++++++++++++++++++----- block.h | 2 ++ savevm.c | 48 +++++++++++++++++++++++++----------------------- 3 files changed, 69 insertions(+), 28 deletions(-)