• M
    savevm: Really verify if a drive supports snapshots · feeee5ac
    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: NMiguel Di Ciurcio Filho <miguel.filho@gmail.com>
    Signed-off-by: NKevin Wolf <kwolf@redhat.com>
    feeee5ac
block.h 9.7 KB