Message ID | 1466152443-15660-3-git-send-email-lma@suse.com |
---|---|
State | New |
Headers | show |
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 > } > >
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); }
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(-)