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