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 <stefa...@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 <up2w...@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 <up2w...@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
