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'
2015-03-06 23:50 GMT+08:00 Stefan Hajnoczi <stefa...@redhat.com>:
> On Fri, Mar 06, 2015 at 10:35:41PM +0800, Yi Wang wrote:
>> Yeah, your method is better. But there is still a little problem.
>> For example: vda has one snapshot with name "s1" and id_str "1", vdb
>> has two snapshots: first name "s1" and id_str "2"; second name "s2"
>> and id_str "3". Error will occur when we want to create snapshot s1,
>> because snapshot s1 with id_str "2" already exists in vdb.
>> So what we only need to do is clearing id_str when name != NULL.
>
> How do you trigger that?
>
> I thought there is already code to prevent this problem. If name="s1"
> then the following code should delete the existing snapshot so it can be
> replaced:
>
> /* Delete old snapshots of the same name */
> if (name && del_existing_snapshots(mon, name) < 0) {
> goto the_end;
> }
>
>> diff --git a/savevm.c b/savevm.c
>> index 08ec678..716d15a 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -1123,6 +1123,14 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>
> Please rebase onto qemu.git/master where do_savevm() has been renamed to
> hmp_savevm().
>
>> if (bdrv_can_snapshot(bs1)) {
>> /* Write VM state size only to the image that contains the
>> state */
>> sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>> +
>> + /* Images may have existing IDs so let the ID be
>> autogenerated if the
>> + * user specify a name.
>> + */
>> + if (name) {
>> + sn->id_str[0] = '\0';
>> + }
>> +
>> ret = bdrv_snapshot_create(bs1, sn);
>> if (ret < 0) {
>> monitor_printf(mon, "Error while creating snapshot on
>> '%s'\n",
>>
>> 2015-03-06 1:40 GMT+08:00 Stefan Hajnoczi <stefa...@redhat.com>:
>> > On Thu, Mar 05, 2015 at 09:05:52PM +0800, Yi Wang wrote:
>> >> Thanks for your reply and Happy Lantern Festival!
>> >> I am afraid you misunderstood what I mean, maybe I didn't express
>> >> clearly :-) My patch works in such case:
>> >> Firstly vm has two disks:
>> >> [root@fox-host vmimg]# virsh domblklist win7
>> >> Target Source
>> >> ------------------------------------------------
>> >> hdb /home/virtio_test.iso
>> >> vda /home/vmimg/win7.img.1
>> >> vdb /home/vmimg/win7.append
>> >>
>> >> Secondly first disk has one snapshot with id_str "1", and another disk
>> >> has three snapshots with id_str "1", "2", "3".
>> >> [root@fox-host vmimg]# qemu-img snapshot -l win7.img.1
>> >> Snapshot list:
>> >> ID TAG VM SIZE DATE VM CLOCK
>> >> 1 s1 0 2015-03-05 10:26:16 00:00:00.000
>> >>
>> >> [root@fox-host vmimg]# qemu-img snapshot -l win7.append
>> >> Snapshot list:
>> >> ID TAG VM SIZE DATE VM CLOCK
>> >> 1 s3 0 2015-03-05 10:29:21 00:00:00.000
>> >> 2 s1 0 2015-03-05 10:29:38 00:00:00.000
>> >> 3 s2 0 2015-03-05 10:30:49 00:00:00.000
>> >>
>> >> In this case, we will fail when create snapshot specifying a name,
>> >> 'cause id_str "2" already exists in disk vdb.
>> >> [root@fox-host8 vmimg]# virsh snapshot-create-as win7-fox s4
>> >> error: operation failed: Failed to take snapshot: Error while creating
>> >> snapshot on 'drive-virtio-disk1'
>> >
>> > This means that name != NULL but it is still unnecessary to duplicate ID
>> > generation.
>> >
>> > Does this work?
>> >
>> > diff --git a/savevm.c b/savevm.c
>> > index 08ec678..e81e4aa 100644
>> > --- a/savevm.c
>> > +++ b/savevm.c
>> > @@ -1047,6 +1047,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>> > QEMUFile *f;
>> > int saved_vm_running;
>> > uint64_t vm_state_size;
>> > + bool generate_ids = true;
>> > qemu_timeval tv;
>> > struct tm tm;
>> > const char *name = qdict_get_try_str(qdict, "name");
>> > @@ -1088,6 +1089,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>> > if (ret >= 0) {
>> > pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>> > pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>> > + generate_ids = false;
>> > } else {
>> > pstrcpy(sn->name, sizeof(sn->name), name);
>> > }
>> > @@ -1123,6 +1125,14 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>> > if (bdrv_can_snapshot(bs1)) {
>> > /* Write VM state size only to the image that contains the
>> > state */
>> > sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>> > +
>> > + /* Images may have existing IDs so let the ID be
>> > autogenerated if the
>> > + * user did not specify a name.
>> > + */
>> > + if (generate_ids) {
>> > + sn->id_str[0] = '\0';
>> > + }
>> > +
>> > ret = bdrv_snapshot_create(bs1, sn);
>> > if (ret < 0) {
>> > monitor_printf(mon, "Error while creating snapshot on
>> > '%s'\n",
>>
>>
>>
>> --
>> Best Regards
>> Yi Wang
--
Best Regards
Yi Wang
