qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] savevm: create snapshot failed when id_str alre


From: Yi Wang
Subject: Re: [Qemu-devel] [PATCH] savevm: create snapshot failed when id_str already exits
Date: Thu, 12 Mar 2015 23:29:38 +0800

How about this?

>From 913cf2cd04167b7f6b892ac1ab405a617d886b97 Mon Sep 17 00:00:00 2001
From: Yi Wang <address@hidden>
Date: Thu, 12 Mar 2015 22:54:42 +0800
Subject: [PATCH] savevm: create snapshot failed when id_str already exists

The command "virsh create" will fail in such condition: vm has two
disks: vda and vdb. vda has snapshot s1 with id "1", vdb doesn't have
s1 but has snapshot s2 with id "1"。When we want to run command "virsh
create s1", del_existing_snapshots() only deletes s1 in vda, and
bdrv_snapshot_create() tries to create vdb's snapshot s1 with id "1",
but id "1" alreay exists in vdb with name "s2"!

The simplest way is call find_new_snapshot_id() unconditionally.

Signed-off-by: Yi Wang <address@hidden>
---
 block/qcow2-snapshot.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5b3903c..cb00f56 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -351,10 +351,8 @@ int qcow2_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)

     memset(sn, 0, sizeof(*sn));

-    /* Generate an ID if it wasn't passed */
-    if (sn_info->id_str[0] == '\0') {
-        find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));
-    }
+    /* Generate an ID */
+    find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));

     /* Check that the ID is unique */
     if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
-- 
1.9.5.msysgit.0


2015-03-11 20:57 GMT+08:00 Stefan Hajnoczi <address@hidden>:
> On Tue, Mar 10, 2015 at 02:48:46PM +0100, Kevin Wolf wrote:
>> Am 10.03.2015 um 14:28 hat Stefan Hajnoczi geschrieben:
>> > On Mon, Mar 09, 2015 at 09:32:47PM +0800, Yi Wang wrote:
>> > > This will trigger the problem: vda has snapshot s1 with id "1", vdb
>> > > doesn't have s1 but has snapshot s2 with id "1"。When we want to run
>> > > command "virsh create s1", del_existing_snapshots() only deletes s1 in
>> > > vda, and bdrv_snapshot_create() tries to create vdb's snapshot s1 with
>> > > id "1", but id "1" alreay exists in vdb with name "s2"!
>> > >
>> > > This shows the condition:
>> > > # qemu-img snapshot -l win7.img.1
>> > > Snapshot list:
>> > > ID TAG VM SIZE DATE VM CLOCK
>> > > 1 s1 534M 2015-03-09 10:28:54 00:03:54.504
>> > >
>> > > # qemu-img snapshot -l win7.append
>> > > Snapshot list:
>> > > ID TAG VM SIZE DATE VM CLOCK
>> > > 1 s2 0 2015-03-05 10:29:21 00:00:00.000
>> > >
>> > > # virsh snapshot-create-as win7 s1
>> > > error: operation failed: Failed to take snapshot: Error while creating
>> > > snapshot on 'drive-virtio-disk1'
>> >
>> > Then we've arrived at changing the behavior of 'savevm' so it always
>> > generates IDs.
>> >
>> > Kevin: What do you think about changing savevm semantics to always
>> > generate IDs?
>>
>> Sounds reasonable. We're free to set whatever ID we want.
>>
>> However, I wouldn't set id_str = "" like in the patch below, but remove
>> the check qcow2_snapshot_create() and call find_new_snapshot_id()
>> unconditionally.
>
> Good idea!
>
> Yi: If you are happy with the approach Kevin suggested, please send a
> patch.
>
> Stefan



-- 
Best Regards
Yi Wang



reply via email to

[Prev in Thread] Current Thread [Next in Thread]