[v2] savevm: Really verify if a drive supports snapshots

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>
---
 block.c   |   21 ++++++++++++++++-----
 block.h   |    1 +
 monitor.c |    5 ++++-
 savevm.c  |   58 ++++++++++++++++++++++++++++++++++++----------------------
 4 files changed, 57 insertions(+), 28 deletions(-)

Am 29.05.2010 21:55, 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>
> ---
>  block.c   |   21 ++++++++++++++++-----
>  block.h   |    1 +
>  monitor.c |    5 ++++-
>  savevm.c  |   58 ++++++++++++++++++++++++++++++++++++----------------------
>  4 files changed, 57 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cd70730..1115a60 100644
> --- a/block.c
> +++ b/block.c
> @@ -1720,15 +1720,26 @@ 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;
> +}
> +
>  int bdrv_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (!drv->bdrv_snapshot_create)
> -        return -ENOTSUP;
> -    return drv->bdrv_snapshot_create(bs, sn_info);
> +    if (bdrv_can_snapshot(bs)) {
> +        return drv->bdrv_snapshot_create(bs, sn_info);
> +    }
> +
> +    return -1;

Not -ENOTSUP as before?

>  }
>  
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block.h b/block.h
> index 24efeb6..fbcd8af 100644
> --- a/block.h
> +++ b/block.h
> @@ -173,6 +173,7 @@ 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_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info);
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/monitor.c b/monitor.c
> index ad50f12..8b4a1d7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2468,8 +2468,11 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>  
>      vm_stop(0);
>  
> -    if (load_vmstate(name) >= 0 && saved_vm_running)
> +    if (load_vmstate(name) >= 0 && saved_vm_running) {
>          vm_start();
> +    } else {
> +        monitor_printf(mon, "Failed to load VM state. VM stopped.\n");
> +    }

If the VM was stopped before, this will print the error message even if
everything went fine.

Kevin

On Mon, May 31, 2010 at 6:59 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>  int bdrv_snapshot_create(BlockDriverState *bs,
>>                           QEMUSnapshotInfo *sn_info)
>>  {
>>      BlockDriver *drv = bs->drv;
>> -    if (!drv)
>> -        return -ENOMEDIUM;
>> -    if (!drv->bdrv_snapshot_create)
>> -        return -ENOTSUP;
>> -    return drv->bdrv_snapshot_create(bs, sn_info);
>> +    if (bdrv_can_snapshot(bs)) {
>> +        return drv->bdrv_snapshot_create(bs, sn_info);
>> +    }
>> +
>> +    return -1;
>
> Not -ENOTSUP as before?

Oops, I will leave as it was before.

>>
>> -    if (load_vmstate(name) >= 0 && saved_vm_running)
>> +    if (load_vmstate(name) >= 0 && saved_vm_running) {
>>          vm_start();
>> +    } else {
>> +        monitor_printf(mon, "Failed to load VM state. VM stopped.\n");
>> +    }
>
> If the VM was stopped before, this will print the error message even if
> everything went fine.

Same here, I will leave as it was before. Luiz is cooking something
for this area AFAIK.

Thanks