Re: [Qemu-devel] [] [PATCH] Show all of snapshot info on every block device in output of 'info snapshots'

[Max Reitz]

On 12.06.2016 17:38, Lin Ma wrote:
> Currently, the output of 'info snapshots' shows fully available snapshots.
> 
> In my opinion there are 2 disadvantages:
> 1. 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.
> 2. It 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 follow Kevin's suggestion, Make the output
> more detailed/accurate:
> (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 | 77 
> +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 3 deletions(-)

I have many comments, but don't worry, it's nothing that can't be fixed.
The overall design looks good to me.

> diff --git a/migration/savevm.c b/migration/savevm.c
> index 6c21231..8444c62 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2153,12 +2153,28 @@ 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;
> -    int nb_sns, i;
> +    bool no_snapshot = true;
> +    int nb_sns, nb_sns_tmp, i;

"nb_sns_tmp" is not a very expressive name. Since you only need to use
it in a for () loop below, I propose moving the declaration there. [2]

>      int total;
>      int *available_snapshots;
>      AioContext *aio_context;
>  
> +    typedef struct SnapshotEntry {
> +        QEMUSnapshotInfo *sn;

I strongly propose not making this a pointer. See [3] for why.

> +        QTAILQ_ENTRY(SnapshotEntry) next;
> +    } SnapshotEntry;
> +
> +    typedef struct ImageEntry {
> +        char *imagename;
> +        QTAILQ_ENTRY(ImageEntry) next;
> +        QTAILQ_HEAD(, SnapshotEntry) snapshots;
> +    } ImageEntry;

I wouldn't declare types inside of a function, but I don't think we
actually have a rule against it.

> +
> +    ImageEntry *image_entry;
> +    SnapshotEntry *snapshot_entry;
> +
>      bs = bdrv_all_find_vmstate_bs();
>      if (!bs) {
>          monitor_printf(mon, "No available block device supports 
> snapshots\n");
> @@ -2175,7 +2191,34 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>          return;
>      }
>  
> -    if (nb_sns == 0) {
> +    QTAILQ_HEAD(image_list, ImageEntry) image_list =
> +        QTAILQ_HEAD_INITIALIZER(image_list);

qemu's coding style mandates declaration of variables at the start of a
block.

> +
> +    for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) {

[2] I think it would make sense to move the declaration of nb_sns_tmp
here and call it something different. Maybe "bs1_nb_sns" or "nb_sns_bs1".

(On a side note: I don't like the name nb_sns in itself and would very
much prefer nb_snapshots, but since this is preexisting, I won't ask you
to change it.)

> +        AioContext *ctx = bdrv_get_aio_context(bs);

Shouldn't this be bs1?

> +
> +        aio_context_acquire(ctx);
> +        nb_sns_tmp = 0;
> +        if (bdrv_can_snapshot(bs1)) {
> +            nb_sns_tmp = bdrv_snapshot_list(bs1, &sn);

I think sn should be initialized to NULL before this call... [1]

> +            if (nb_sns_tmp > 0) {
> +                no_snapshot = false;
> +                ImageEntry *ie = g_malloc0(sizeof(*ie));

First: Declaration needs to be done at the start of the block.

Second: While not wrong, you may want to use g_new0(ImageEntry, 1)
instead. The benefit of using g_new0() is that it will return the
correct type.

> +                ie->imagename = g_strdup(bdrv_get_device_name(bs1));

I'm not sure why you're using g_strdup() here. Wouldn't it suffice to
make imagename a const char * and drop the g_strdup()?

> +                QTAILQ_INIT(&ie->snapshots);
> +                QTAILQ_INSERT_TAIL(&image_list, ie, next);
> +                int x;

First: This needs to be declared at the start of this block.

Second: I don't particularly like "x" as the name of this variable.
Normally, iterator variables are named i, j, k, .... Since "i" is taken
already, I'd propose using "j" for this one.

Third: Since "i" is not used here at all, you could actually just use
"i" for the following loop. This is what I prefer.

> +                for (x = 0; x < nb_sns_tmp; x++) {
> +                    SnapshotEntry *se = g_malloc0(sizeof(*se));

Same here, I'd propose using g_new0(SnapshotEntry, 1).

> +                    se->sn = &sn[x];

(Note that if you followed my proposal above of making se->sn not a
pointer, this should be se->sn = sn[x].)

> +                    QTAILQ_INSERT_TAIL(&ie->snapshots, se, next);
> +                }
> +            }

[1] ...and then sn needs to be freed here (using g_free()).

(This is why it needs to be initialized to NULL, so g_free() will work
regardless of whether bdrv_snapshot_list() wrote something to sn.)

> +        }
> +        aio_context_release(ctx);
> +    }
> +
> +    if (no_snapshot) {
>          monitor_printf(mon, "There is no snapshot available.\n");
>          return;
>      }
> @@ -2183,17 +2226,28 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>      available_snapshots = g_new0(int, nb_sns);

It might make sense to rename this variable. It's currently named this
way because everything that's not entered into this array is an
"unavailable" snapshot, and will not be displayed.

You are changing this behavior, and this array will simply contain all
the snapshot indices that are globally available on all BDSs. I think
renaming it to "global_snapshots" may make sense.

But since this is a preexisting name, it's up to you whether you want to
rename it or not.

>      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) {

I think this should be in an independent patch because this is an
independent fix.

>              available_snapshots[total] = i;
>              total++;
>          }
>      }
>  
> +    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]];
> +            QTAILQ_FOREACH(image_entry, &image_list, next) {
> +                QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, 
> next) {

This must be QTAILQ_FOREACH_SAFE().

> +                    if (!strcmp(sn->name, snapshot_entry->sn->name)) {
> +                        QTAILQ_REMOVE(&image_entry->snapshots, 
> snapshot_entry,
> +                                      next);

[3] You are leaking snapshot_entry->sn here. It's rather difficult to
avoid this if you want to keep that field a pointer. Therefore, I
proposed not making it a pointer.

Also, you are leaking snapshot_entry itself here. Easy fix, g_free() it
and use QTAILQ_FOREACH_SAFE().

> +                    }
> +                }
> +            }

I think this should be done in the for () loop above (the "for (i < 0; i
< nb_sns; i++)" loop).

This is because I think we should separate code for outputting
information and code for gathering/filtering this information. The code
added here falls in the latter category, while the existing code here is
just for outputting what we found.

> +            pstrcpy(sn->id_str, sizeof(sn->id_str), "--");

This pstrcpy() warrants a comment, like "The ID is not guaranteed to be
the same on all images, so overwrite it". Note that this pstrcpy() call
addition should be in the same patch as the s/\.id_str/.name/ in the
bdrv_all_find_snapshot() call above.

>              bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
>              monitor_printf(mon, "\n");
>          }
> @@ -2201,6 +2255,23 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>          monitor_printf(mon, "There is no suitable snapshot available\n");

This message should now be rephrased to something like simply
"(none)\n", because the new "List of snapshots present on all disks:"
headline will fully explain what that means.

>      }

Nit pick: The following code will always leave an empty line after
everything. I think that's superfluous, and it can be amended as follows
(if you want to amend it, that is; if you really like that empty line,
then feel free to disregard my suggestion):

> +    monitor_printf(mon, "\n");

Drop this.

> +    QTAILQ_FOREACH(image_entry, &image_list, next) {
> +        if (QTAILQ_EMPTY(&image_entry->snapshots)) {
> +            continue;
> +        }

Put monitor_printf(mon, "\n"); here.

> +        monitor_printf(mon, "List of partial (non-loadable) snapshots on 
> '%s':",
> +                       image_entry->imagename);
> +        monitor_printf(mon, "\n");

(Why did you not concatenate these two strings in a single
monitor_printf() call?)

> +        bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
> +        monitor_printf(mon, "\n");

Drop this.

> +        QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {

Put monitor_printf(mon, "\n"); here.

> +            bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
> +                               snapshot_entry->sn);
> +            monitor_printf(mon, "\n");

And drop this. Again, the suggestions on moving the
monitor_printf(mon, "\n"); calls around are just suggestions, and it's
up to you whether you want to follow them or not.

> +        }
> +    }
> +

You're leaking all entries in the image_list here, and subsequently all
snapshots in the snapshots list of each image, and also the imagename of
each image_list entry. The latter wouldn't occur if you made imagename a
const char * and drop the g_strdup() when assigning is, as I have
suggested somewhere above.

>      g_free(sn_tab);
>      g_free(available_snapshots);
>  
> 

Despite of the many comments I had, the overall design looks good to me.

Max

signature.asc
Description: OpenPGP digital signature