[2/2,V2] hmp: show all of snapshot info on every block dev in output of 'info 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(-)

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

>  }
>  
>