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

[Stefan Hajnoczi]

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

pgpF34Z0Eyb9o.pgp
Description: PGP signature