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

2015-02-25 17:41 GMT+08:00 Stefan Hajnoczi <address@hidden>:
> 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 <address@hidden>
>> 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 <address@hidden>
>> ---
>>  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