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 <l...@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
> }
>
>
signature.asc
Description: OpenPGP digital signature
