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: Mon, 9 Mar 2015 21:32:47 +0800

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 <address@hidden>:
> 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 <address@hidden>:
>> > 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:
>> >> address@hidden 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".
>> >> address@hidden 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
>> >>
>> >> address@hidden 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.
>> >> address@hidden 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



reply via email to

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