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

[Stefan Hajnoczi <stefanha@redhat.com>]

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