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

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

[Yi Wang]

>From b119164ba6715f53594facfc4d1022c198852e9d Mon Sep 17 00:00:00 2001
From: Yi Wang <up2wing@gmail.com>
Date: Mon, 12 Jan 2015 00:05:40 +0800
Subject: [PATCH] savevm: create snapshot failed when id_str already exits

Create snapshot failed in this case:
1) vm has two or more qcow2 disks;
2) the first disk has snapshot with id_str 1, and the second disk has
snapshots with id_str 2 and 3, for example;
3) create snapshot using virsh snapshot-create/snapshot-create-as will
fail, 'cause id_str 2 has already existed in disk two.

The reason is that do_savevm() didn't check id_str before create
bdrv_snapshot_create(), and this patch fixed this.

Signed-off-by: Yi Wang <up2wing@gmail.com>
---
 block/snapshot.c         |   32 ++++++++++++++++++++++++++++++++
 include/block/snapshot.h |    1 +
 savevm.c                 |    7 +++++++
 3 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 698e1a1..f2757ab 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -66,6 +66,38 @@ int bdrv_snapshot_find(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info,
     return ret;
 }

+int bdrv_snapshot_get_id_str(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i, ret;
+    int max = 0, temp_max;
+    bool need_max = false;
+
+    ret = -ENOENT;
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        return ret;
+    }
+
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        temp_max = atoi(sn->id_str);
+        if (max < temp_max) {
+            max = temp_max;
+        }
+        if (!need_max && !strcmp(sn->id_str, sn_info->id_str)) {
+            need_max = true;
+        }
+        if (need_max) {
+            snprintf(sn_info->id_str, 128*sizeof(char), "%d", max+1);
+        }
+
+    }
+    g_free(sn_tab);
+    ret = 0;
+    return ret;
+}
+
 /**
  * Look up an internal snapshot by @id and @name.
  * @bs: block device to search
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 770d9bb..047ed7b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -49,6 +49,7 @@ typedef struct QEMUSnapshotInfo {

 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *name);
+int bdrv_snapshot_get_id_str(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
 bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
                                        const char *id,
                                        const char *name,
diff --git a/savevm.c b/savevm.c
index 08ec678..f2edc13 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1123,6 +1123,13 @@ 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);
+
+            /* To avoid sn->id_str already exists */
+            if (bdrv_snapshot_get_id_str(bs1, sn) < 0) {
+                monitor_printf(mon, "Error when get id str.\n");
+                goto the_end;
+            }
+
             ret = bdrv_snapshot_create(bs1, sn);
             if (ret < 0) {
                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-- 
1.7.1


-- 
Best Regards
Yi Wang

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

[Amit Shah]

On (Mon) 12 Jan 2015 [01:12:41], Yi Wang wrote:
> From b119164ba6715f53594facfc4d1022c198852e9d Mon Sep 17 00:00:00 2001
> From: Yi Wang <up2wing@gmail.com>
> Date: Mon, 12 Jan 2015 00:05:40 +0800
> Subject: [PATCH] savevm: create snapshot failed when id_str already exits
> 
> Create snapshot failed in this case:
> 1) vm has two or more qcow2 disks;
> 2) the first disk has snapshot with id_str 1, and the second disk has
> snapshots with id_str 2 and 3, for example;
> 3) create snapshot using virsh snapshot-create/snapshot-create-as will
> fail, 'cause id_str 2 has already existed in disk two.
> 
> The reason is that do_savevm() didn't check id_str before create
> bdrv_snapshot_create(), and this patch fixed this.
> 
> Signed-off-by: Yi Wang <up2wing@gmail.com>

I'll defer to the block people on this patch.

		Amit

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3758 bytes --]

On Mon, Jan 12, 2015 at 01:12:41AM +0800, Yi Wang wrote:
> From b119164ba6715f53594facfc4d1022c198852e9d Mon Sep 17 00:00:00 2001
> From: Yi Wang <up2wing@gmail.com>
> Date: Mon, 12 Jan 2015 00:05:40 +0800
> Subject: [PATCH] savevm: create snapshot failed when id_str already exits
> 
> Create snapshot failed in this case:
> 1) vm has two or more qcow2 disks;
> 2) the first disk has snapshot with id_str 1, and the second disk has
> snapshots with id_str 2 and 3, for example;
> 3) create snapshot using virsh snapshot-create/snapshot-create-as will
> fail, 'cause id_str 2 has already existed in disk two.
> 
> The reason is that do_savevm() didn't check id_str before create
> bdrv_snapshot_create(), and this patch fixed this.
> 
> Signed-off-by: Yi Wang <up2wing@gmail.com>
> ---
>  block/snapshot.c         |   32 ++++++++++++++++++++++++++++++++
>  include/block/snapshot.h |    1 +
>  savevm.c                 |    7 +++++++
>  3 files changed, 40 insertions(+), 0 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 698e1a1..f2757ab 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -66,6 +66,38 @@ int bdrv_snapshot_find(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info,
>      return ret;
>  }
> 
> +int bdrv_snapshot_get_id_str(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> +{
> +    QEMUSnapshotInfo *sn_tab, *sn;
> +    int nb_sns, i, ret;
> +    int max = 0, temp_max;
> +    bool need_max = false;
> +
> +    ret = -ENOENT;
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    if (nb_sns < 0) {
> +        return ret;
> +    }
> +
> +    for (i = 0; i < nb_sns; i++) {
> +        sn = &sn_tab[i];
> +        temp_max = atoi(sn->id_str);
> +        if (max < temp_max) {
> +            max = temp_max;
> +        }
> +        if (!need_max && !strcmp(sn->id_str, sn_info->id_str)) {
> +            need_max = true;
> +        }
> +        if (need_max) {
> +            snprintf(sn_info->id_str, 128*sizeof(char), "%d", max+1);
> +        }
> +
> +    }
> +    g_free(sn_tab);
> +    ret = 0;
> +    return ret;
> +}
> +
>  /**
>   * Look up an internal snapshot by @id and @name.
>   * @bs: block device to search
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index 770d9bb..047ed7b 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -49,6 +49,7 @@ typedef struct QEMUSnapshotInfo {
> 
>  int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                         const char *name);
> +int bdrv_snapshot_get_id_str(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
>  bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>                                         const char *id,
>                                         const char *name,
> diff --git a/savevm.c b/savevm.c
> index 08ec678..f2edc13 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1123,6 +1123,13 @@ 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);
> +
> +            /* To avoid sn->id_str already exists */
> +            if (bdrv_snapshot_get_id_str(bs1, sn) < 0) {
> +                monitor_printf(mon, "Error when get id str.\n");
> +                goto the_end;
> +            }
> +

Does this solve the issue?

/* Images may have existing IDs so let the ID be autogenerated if the
 * user did not specify a name.
 */
if (!name) {
    sn->id_str[0] = '\0';
}

The effect is similar to your patch but it avoids duplicating the id_str
generation.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Yi Wang]

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'

2015-02-25 17:41 GMT+08:00 Stefan Hajnoczi <stefanha@redhat.com>:
> On Mon, Jan 12, 2015 at 01:12:41AM +0800, Yi Wang wrote:
>> From b119164ba6715f53594facfc4d1022c198852e9d Mon Sep 17 00:00:00 2001
>> From: Yi Wang <up2wing@gmail.com>
>> Date: Mon, 12 Jan 2015 00:05:40 +0800
>> Subject: [PATCH] savevm: create snapshot failed when id_str already exits
>>
>> Create snapshot failed in this case:
>> 1) vm has two or more qcow2 disks;
>> 2) the first disk has snapshot with id_str 1, and the second disk has
>> snapshots with id_str 2 and 3, for example;
>> 3) create snapshot using virsh snapshot-create/snapshot-create-as will
>> fail, 'cause id_str 2 has already existed in disk two.
>>
>> The reason is that do_savevm() didn't check id_str before create
>> bdrv_snapshot_create(), and this patch fixed this.
>>
>> Signed-off-by: Yi Wang <up2wing@gmail.com>
>> ---
>>  block/snapshot.c         |   32 ++++++++++++++++++++++++++++++++
>>  include/block/snapshot.h |    1 +
>>  savevm.c                 |    7 +++++++
>>  3 files changed, 40 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 698e1a1..f2757ab 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -66,6 +66,38 @@ int bdrv_snapshot_find(BlockDriverState *bs,
>> QEMUSnapshotInfo *sn_info,
>>      return ret;
>>  }
>>
>> +int bdrv_snapshot_get_id_str(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>> +{
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i, ret;
>> +    int max = 0, temp_max;
>> +    bool need_max = false;
>> +
>> +    ret = -ENOENT;
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        return ret;
>> +    }
>> +
>> +    for (i = 0; i < nb_sns; i++) {
>> +        sn = &sn_tab[i];
>> +        temp_max = atoi(sn->id_str);
>> +        if (max < temp_max) {
>> +            max = temp_max;
>> +        }
>> +        if (!need_max && !strcmp(sn->id_str, sn_info->id_str)) {
>> +            need_max = true;
>> +        }
>> +        if (need_max) {
>> +            snprintf(sn_info->id_str, 128*sizeof(char), "%d", max+1);
>> +        }
>> +
>> +    }
>> +    g_free(sn_tab);
>> +    ret = 0;
>> +    return ret;
>> +}
>> +
>>  /**
>>   * Look up an internal snapshot by @id and @name.
>>   * @bs: block device to search
>> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
>> index 770d9bb..047ed7b 100644
>> --- a/include/block/snapshot.h
>> +++ b/include/block/snapshot.h
>> @@ -49,6 +49,7 @@ typedef struct QEMUSnapshotInfo {
>>
>>  int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>                         const char *name);
>> +int bdrv_snapshot_get_id_str(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
>>  bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>>                                         const char *id,
>>                                         const char *name,
>> diff --git a/savevm.c b/savevm.c
>> index 08ec678..f2edc13 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -1123,6 +1123,13 @@ 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);
>> +
>> +            /* To avoid sn->id_str already exists */
>> +            if (bdrv_snapshot_get_id_str(bs1, sn) < 0) {
>> +                monitor_printf(mon, "Error when get id str.\n");
>> +                goto the_end;
>> +            }
>> +
>
> Does this solve the issue?
>
> /* Images may have existing IDs so let the ID be autogenerated if the
>  * user did not specify a name.
>  */
> if (!name) {
>     sn->id_str[0] = '\0';
> }
>
> The effect is similar to your patch but it avoids duplicating the id_str
> generation.



-- 
Best Regards
Yi Wang

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]

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",

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Yi Wang]

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.

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)
         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 <stefanha@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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4838 bytes --]

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 <stefanha@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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Yi Wang]

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 <stefanha@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 <stefanha@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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

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?

diff --git a/savevm.c b/savevm.c
index ce2b6a2..bee2143 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1141,7 +1141,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         ret = bdrv_snapshot_find(bs, old_sn, name);
         if (ret >= 0) {
             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
-            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
         } else {
             pstrcpy(sn->name, sizeof(sn->name), name);
         }
@@ -1178,6 +1177,12 @@ void hmp_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);
+
+            /* Force ID generation for each image since there could be naming
+             * collisions.
+             */
+            sn->id_str[0] = '\0';
+
             ret = bdrv_snapshot_create(bs1, sn);
             if (ret < 0) {
                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

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.

Kevin

> diff --git a/savevm.c b/savevm.c
> index ce2b6a2..bee2143 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1141,7 +1141,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>          ret = bdrv_snapshot_find(bs, old_sn, name);
>          if (ret >= 0) {
>              pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> -            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>          } else {
>              pstrcpy(sn->name, sizeof(sn->name), name);
>          }
> @@ -1178,6 +1177,12 @@ void hmp_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);
> +
> +            /* Force ID generation for each image since there could be naming
> +             * collisions.
> +             */
> +            sn->id_str[0] = '\0';
> +
>              ret = bdrv_snapshot_create(bs1, sn);
>              if (ret < 0) {
>                  monitor_printf(mon, "Error while creating snapshot on '%s'\n",



[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Yi Wang]

How about this?

>From 913cf2cd04167b7f6b892ac1ab405a617d886b97 Mon Sep 17 00:00:00 2001
From: Yi Wang <up2wing@gmail.com>
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 <up2wing@gmail.com>
---
 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 <stefanha@gmail.com>:
> 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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

On Thu, Mar 12, 2015 at 11:29:38PM +0800, Yi Wang wrote:
> How about this?
> 
> From 913cf2cd04167b7f6b892ac1ab405a617d886b97 Mon Sep 17 00:00:00 2001
> From: Yi Wang <up2wing@gmail.com>
> 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 <up2wing@gmail.com>
> ---
>  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

Thanks, I've applied the patch to the block-next branch for QEMU 2.4:
https://github.com/stefanha/qemu/commits/block-next

Please send future patches as separate top-level email threads using
git-send-email(1).  I missed this reply since it was not a new email
thread.  git-am(1) refuses to apply the patch because of line-wrapping
in your email, I had to fix that manually.

Also, please stick to ASCII characters when possible.  Your patch
description uses the Chinese full stop (句號).  I had to manually edit
the patch because git-am(1) refuses to apply a copy-pasted patch without
explicit character encoding information.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]