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.
pgp9IRMQm767S.pgp
Description: PGP signature
