[Qemu-devel] Re: [PATCH v5] savevm: Really verify if a drive supports snapshots

[Kevin Wolf <kwolf@redhat.com>]

Am 08.06.2010 15:40, 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() 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.filho@gmail.com>

Thanks, I updated the patch in the block branch. Let's hope we've got it
correct this time.

Kevin