| Message ID | CAN35MuS=k1=GKqjCsTugVvYLgAErkV_s4T=H5F8q2qZ=KyNXaA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On (Mon) 12 Jan 2015 [01:12:41], 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> I'll defer to the block people on this patch. Amit
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.
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 <stefanha@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 <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.
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; + } + ret = bdrv_snapshot_create(bs1, sn); if (ret < 0) { monitor_printf(mon, "Error while creating snapshot on '%s'\n",