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

[Markus Armbruster]

Kevin Wolf <address@hidden> 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 <address@hidden>
>
> 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.

[...]