Re: [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots'

[Max Reitz]

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 <address@hidden>
> ---
>  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