* [Qemu-devel] [PATCH 0/2 V2] Show all of snapshot info on every block device in output of 'info snapshots'
@ 2016-06-17 8:34 Lin Ma
2016-06-17 8:34 ` [Qemu-devel] [PATCH 1/2 V2] hmp: use snapshot name to determine whether a snapshot is 'fully available' Lin Ma
2016-06-17 8:34 ` [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' Lin Ma
0 siblings, 2 replies; 5+ messages in thread
From: Lin Ma @ 2016-06-17 8:34 UTC (permalink / raw)
To: mreitz, kwolf, qemu-devel
V2: Split it to 2 patches.
Lin Ma (2):
hmp: use snapshot name to determine whether a snapshot is 'fully
available'
hmp: show all of snapshot info on every block dev in output of 'info
snapshots'
migration/savevm.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 88 insertions(+), 8 deletions(-)
--
2.8.1
^ permalink raw reply [flat|nested] 5+ messages in thread* [Qemu-devel] [PATCH 1/2 V2] hmp: use snapshot name to determine whether a snapshot is 'fully available' 2016-06-17 8:34 [Qemu-devel] [PATCH 0/2 V2] Show all of snapshot info on every block device in output of 'info snapshots' Lin Ma @ 2016-06-17 8:34 ` Lin Ma 2016-06-27 16:35 ` Max Reitz 2016-06-17 8:34 ` [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' Lin Ma 1 sibling, 1 reply; 5+ messages in thread From: Lin Ma @ 2016-06-17 8:34 UTC (permalink / raw) To: mreitz, kwolf, qemu-devel Currently qemu uses snapshot id to determine whether a snapshot is fully available, It causes incorrect output in some scenario. For instance: (qemu) info block drive_image1 (#block113): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk0.qcow2 (qcow2) Cache mode: writeback drive_image2 (#block349): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk1.qcow2 (qcow2) Cache mode: writeback (qemu) (qemu) info snapshots There is no snapshot available. (qemu) (qemu) snapshot_blkdev_internal drive_image1 snap1 (qemu) (qemu) info snapshots There is no suitable snapshot available (qemu) (qemu) savevm checkpoint-1 (qemu) (qemu) info snapshots ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 (qemu) $ qemu-img snapshot -l disk0.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 2 checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 $ qemu-img snapshot -l disk1.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 checkpoint-1 0 2016-05-22 16:58:07 00:02:06.813 The patch uses snapshot name instead of snapshot id to determine whether a snapshot is fully available and uses '--' instead of snapshot id in output because the snapshot id is not guaranteed to be the same on all images. For instance: (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 Signed-off-by: Lin Ma <lma@suse.com> --- migration/savevm.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index ae2ef8b..0c4e0d9 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2220,7 +2220,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) available_snapshots = g_new0(int, nb_sns); total = 0; for (i = 0; i < nb_sns; i++) { - if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) { + if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { available_snapshots[total] = i; total++; } @@ -2231,6 +2231,10 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) monitor_printf(mon, "\n"); for (i = 0; i < total; i++) { sn = &sn_tab[available_snapshots[i]]; + /* The ID is not guaranteed to be the same on all images, so + * overwrite it. + */ + pstrcpy(sn->id_str, sizeof(sn->id_str), "--"); bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); monitor_printf(mon, "\n"); } -- 2.8.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2 V2] hmp: use snapshot name to determine whether a snapshot is 'fully available' 2016-06-17 8:34 ` [Qemu-devel] [PATCH 1/2 V2] hmp: use snapshot name to determine whether a snapshot is 'fully available' Lin Ma @ 2016-06-27 16:35 ` Max Reitz 0 siblings, 0 replies; 5+ messages in thread From: Max Reitz @ 2016-06-27 16:35 UTC (permalink / raw) To: Lin Ma, kwolf, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2068 bytes --] On 17.06.2016 10:34, Lin Ma wrote: > Currently qemu uses snapshot id to determine whether a snapshot is fully > available, It causes incorrect output in some scenario. > > For instance: > (qemu) info block > drive_image1 (#block113): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk0.qcow2 > (qcow2) > Cache mode: writeback > > drive_image2 (#block349): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk1.qcow2 > (qcow2) > Cache mode: writeback > (qemu) > (qemu) info snapshots > There is no snapshot available. > (qemu) > (qemu) snapshot_blkdev_internal drive_image1 snap1 > (qemu) > (qemu) info snapshots > There is no suitable snapshot available > (qemu) > (qemu) savevm checkpoint-1 > (qemu) > (qemu) info snapshots > ID TAG VM SIZE DATE VM CLOCK > 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 > (qemu) > > $ qemu-img snapshot -l disk0.qcow2 > Snapshot list: > ID TAG VM SIZE DATE VM CLOCK > 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 > 2 checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 > > $ qemu-img snapshot -l disk1.qcow2 > Snapshot list: > ID TAG VM SIZE DATE VM CLOCK > 1 checkpoint-1 0 2016-05-22 16:58:07 00:02:06.813 > > The patch uses snapshot name instead of snapshot id to determine whether a > snapshot is fully available and uses '--' instead of snapshot id in output > because the snapshot id is not guaranteed to be the same on all images. > For instance: > (qemu) info snapshots > List of snapshots present on all disks: > ID TAG VM SIZE DATE VM CLOCK > -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 > > Signed-off-by: Lin Ma <lma@suse.com> > --- > migration/savevm.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 498 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' 2016-06-17 8:34 [Qemu-devel] [PATCH 0/2 V2] Show all of snapshot info on every block device in output of 'info snapshots' Lin Ma 2016-06-17 8:34 ` [Qemu-devel] [PATCH 1/2 V2] hmp: use snapshot name to determine whether a snapshot is 'fully available' Lin Ma @ 2016-06-17 8:34 ` Lin Ma 2016-06-27 17:21 ` Max Reitz 1 sibling, 1 reply; 5+ messages in thread From: Lin Ma @ 2016-06-17 8:34 UTC (permalink / raw) To: mreitz, kwolf, qemu-devel Currently, the output of 'info snapshots' shows fully available snapshots. It's opaque, hides some snapshot information to users. It's not convenient if users want to know more about all of snapshot information on every block device via monitor. Follow Kevin's and Max's proposals, The patch make the output more detailed: (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 List of partial (non-loadable) snapshots on 'drive_image1': ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 Signed-off-by: Lin Ma <lma@suse.com> --- migration/savevm.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 83 insertions(+), 7 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 0c4e0d9..b78e37e 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2190,12 +2190,31 @@ void hmp_delvm(Monitor *mon, const QDict *qdict) void hmp_info_snapshots(Monitor *mon, const QDict *qdict) { BlockDriverState *bs, *bs1; + BdrvNextIterator it1; QEMUSnapshotInfo *sn_tab, *sn; + bool no_snapshot = true; int nb_sns, i; int total; - int *available_snapshots; + int *global_snapshots; AioContext *aio_context; + typedef struct SnapshotEntry { + QEMUSnapshotInfo sn; + QTAILQ_ENTRY(SnapshotEntry) next; + } SnapshotEntry; + + typedef struct ImageEntry { + const char *imagename; + QTAILQ_ENTRY(ImageEntry) next; + QTAILQ_HEAD(, SnapshotEntry) snapshots; + } ImageEntry; + + QTAILQ_HEAD(image_list, ImageEntry) image_list = + QTAILQ_HEAD_INITIALIZER(image_list); + + ImageEntry *image_entry; + SnapshotEntry *snapshot_entry; + bs = bdrv_all_find_vmstate_bs(); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); @@ -2212,25 +2231,65 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) return; } - if (nb_sns == 0) { + for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) { + int bs1_nb_sns = 0; + ImageEntry *ie; + SnapshotEntry *se; + AioContext *ctx = bdrv_get_aio_context(bs1); + + aio_context_acquire(ctx); + if (bdrv_can_snapshot(bs1)) { + sn = NULL; + bs1_nb_sns = bdrv_snapshot_list(bs1, &sn); + if (bs1_nb_sns > 0) { + no_snapshot = false; + ie = g_new0(ImageEntry, 1); + ie->imagename = bdrv_get_device_name(bs1); + QTAILQ_INIT(&ie->snapshots); + QTAILQ_INSERT_TAIL(&image_list, ie, next); + for (i = 0; i < bs1_nb_sns; i++) { + se = g_new0(SnapshotEntry, 1); + se->sn = sn[i]; + QTAILQ_INSERT_TAIL(&ie->snapshots, se, next); + } + } + g_free(sn); + } + aio_context_release(ctx); + } + + if (no_snapshot) { monitor_printf(mon, "There is no snapshot available.\n"); return; } - available_snapshots = g_new0(int, nb_sns); + global_snapshots = g_new0(int, nb_sns); total = 0; for (i = 0; i < nb_sns; i++) { + SnapshotEntry *tmp; if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { - available_snapshots[total] = i; + global_snapshots[total] = i; total++; + QTAILQ_FOREACH(image_entry, &image_list, next) { + QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, + next, tmp) { + if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) { + QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry, + next); + g_free(snapshot_entry); + } + } + } } } + monitor_printf(mon, "List of snapshots present on all disks:\n"); + if (total > 0) { bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); monitor_printf(mon, "\n"); for (i = 0; i < total; i++) { - sn = &sn_tab[available_snapshots[i]]; + sn = &sn_tab[global_snapshots[i]]; /* The ID is not guaranteed to be the same on all images, so * overwrite it. */ @@ -2239,11 +2298,28 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) monitor_printf(mon, "\n"); } } else { - monitor_printf(mon, "There is no suitable snapshot available\n"); + monitor_printf(mon, "None\n"); + } + + QTAILQ_FOREACH(image_entry, &image_list, next) { + if (QTAILQ_EMPTY(&image_entry->snapshots)) { + continue; + } + monitor_printf(mon, "\n"); + monitor_printf(mon, "List of partial (non-loadable) snapshots on '%s':" + "\n", + image_entry->imagename); + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); + monitor_printf(mon, "\n"); + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, + &snapshot_entry->sn); + monitor_printf(mon, "\n"); + } } g_free(sn_tab); - g_free(available_snapshots); + g_free(global_snapshots); } -- 2.8.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' 2016-06-17 8:34 ` [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' Lin Ma @ 2016-06-27 17:21 ` Max Reitz 0 siblings, 0 replies; 5+ messages in thread From: Max Reitz @ 2016-06-27 17:21 UTC (permalink / raw) To: Lin Ma, kwolf, qemu-devel [-- Attachment #1: Type: text/plain, Size: 7055 bytes --] On 17.06.2016 10:34, Lin Ma wrote: > Currently, the output of 'info snapshots' shows fully available snapshots. > It's opaque, hides some snapshot information to users. It's not convenient > if users want to know more about all of snapshot information on every block > device via monitor. > > Follow Kevin's and Max's proposals, The patch make the output more detailed: > (qemu) info snapshots > List of snapshots present on all disks: > ID TAG VM SIZE DATE VM CLOCK > -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 > > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOCK > 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 > > Signed-off-by: Lin Ma <lma@suse.com> > --- > migration/savevm.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 83 insertions(+), 7 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 0c4e0d9..b78e37e 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2190,12 +2190,31 @@ void hmp_delvm(Monitor *mon, const QDict *qdict) > void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > { > BlockDriverState *bs, *bs1; > + BdrvNextIterator it1; > QEMUSnapshotInfo *sn_tab, *sn; > + bool no_snapshot = true; > int nb_sns, i; > int total; > - int *available_snapshots; > + int *global_snapshots; > AioContext *aio_context; > > + typedef struct SnapshotEntry { > + QEMUSnapshotInfo sn; > + QTAILQ_ENTRY(SnapshotEntry) next; > + } SnapshotEntry; > + > + typedef struct ImageEntry { > + const char *imagename; > + QTAILQ_ENTRY(ImageEntry) next; > + QTAILQ_HEAD(, SnapshotEntry) snapshots; > + } ImageEntry; > + > + QTAILQ_HEAD(image_list, ImageEntry) image_list = I think this should be just QTAILQ_HEAD(, ImageEntry). There's no need to name the type. > + QTAILQ_HEAD_INITIALIZER(image_list); > + > + ImageEntry *image_entry; > + SnapshotEntry *snapshot_entry; > + > bs = bdrv_all_find_vmstate_bs(); > if (!bs) { > monitor_printf(mon, "No available block device supports snapshots\n"); > @@ -2212,25 +2231,65 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > return; > } > > - if (nb_sns == 0) { > + for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) { > + int bs1_nb_sns = 0; > + ImageEntry *ie; > + SnapshotEntry *se; > + AioContext *ctx = bdrv_get_aio_context(bs1); > + > + aio_context_acquire(ctx); > + if (bdrv_can_snapshot(bs1)) { > + sn = NULL; > + bs1_nb_sns = bdrv_snapshot_list(bs1, &sn); > + if (bs1_nb_sns > 0) { > + no_snapshot = false; > + ie = g_new0(ImageEntry, 1); > + ie->imagename = bdrv_get_device_name(bs1); > + QTAILQ_INIT(&ie->snapshots); > + QTAILQ_INSERT_TAIL(&image_list, ie, next); > + for (i = 0; i < bs1_nb_sns; i++) { > + se = g_new0(SnapshotEntry, 1); > + se->sn = sn[i]; > + QTAILQ_INSERT_TAIL(&ie->snapshots, se, next); > + } > + } > + g_free(sn); > + } > + aio_context_release(ctx); > + } > + > + if (no_snapshot) { > monitor_printf(mon, "There is no snapshot available.\n"); > return; > } > > - available_snapshots = g_new0(int, nb_sns); > + global_snapshots = g_new0(int, nb_sns); > total = 0; > for (i = 0; i < nb_sns; i++) { > + SnapshotEntry *tmp; That is not a very expressive variable name. I'd call it next_sn or something like that, even if you don't actively use it. > if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { > - available_snapshots[total] = i; > + global_snapshots[total] = i; > total++; > + QTAILQ_FOREACH(image_entry, &image_list, next) { > + QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, > + next, tmp) { > + if (!strcmp(sn_tab[i].name, snapshot_entry->sn.name)) { > + QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry, > + next); In case a line needs to be wrapped in the middle of a function call (or macro) parameter list, the following lines should be aligned to the opening parenthesis (as you have done for bdrv_snapshot_dump() below). > + g_free(snapshot_entry); > + } > + } > + } > } > } > > + monitor_printf(mon, "List of snapshots present on all disks:\n"); > + > if (total > 0) { > bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); > monitor_printf(mon, "\n"); > for (i = 0; i < total; i++) { > - sn = &sn_tab[available_snapshots[i]]; > + sn = &sn_tab[global_snapshots[i]]; > /* The ID is not guaranteed to be the same on all images, so > * overwrite it. > */ > @@ -2239,11 +2298,28 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "\n"); > } > } else { > - monitor_printf(mon, "There is no suitable snapshot available\n"); > + monitor_printf(mon, "None\n"); > + } > + > + QTAILQ_FOREACH(image_entry, &image_list, next) { > + if (QTAILQ_EMPTY(&image_entry->snapshots)) { > + continue; > + } > + monitor_printf(mon, "\n"); > + monitor_printf(mon, "List of partial (non-loadable) snapshots on '%s':" > + "\n", > + image_entry->imagename); Sorry that I'm nagging about this again, but I'd again combine these consecutive monitor_printf()s to: monitor_printf(mon, "\nList of partial (non-loadable) snapshots on '%s':\n", image_entry->imagename); > + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); > + monitor_printf(mon, "\n"); > + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { > + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, > + &snapshot_entry->sn); > + monitor_printf(mon, "\n"); > + } > } > > g_free(sn_tab); > - g_free(available_snapshots); > + g_free(global_snapshots); > You're still leaking the image_list and thus all snapshot lists here. All of the entries in image_list and all of the entries in their ImageEntry.snapshots lists still need to be freed. Max > } > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 498 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-27 17:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-17 8:34 [Qemu-devel] [PATCH 0/2 V2] Show all of snapshot info on every block device in output of 'info snapshots' Lin Ma 2016-06-17 8:34 ` [Qemu-devel] [PATCH 1/2 V2] hmp: use snapshot name to determine whether a snapshot is 'fully available' Lin Ma 2016-06-27 16:35 ` Max Reitz 2016-06-17 8:34 ` [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' Lin Ma 2016-06-27 17:21 ` Max Reitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).