[Qemu-devel] [PATCH 0/2 V4] Show all of snapshot info on every block dev

[Qemu-devel] [PATCH 0/2 V4] Show all of snapshot info on every block dev

[Lin Ma]

V4: Use QTAILQ_FOREACH_SAFE instead of QTAILQ_FOREACH while freeing snapshot lists.

V3: Fix leaking the entries in image_list and the entries in their
    ImageEntry.snapshots lists

V2: Split it to 2 patches.

Lin Ma (2):
  hmp: use snapshot name to determine whether a snapshot is 'fully
    available'
  hmp: show all of snapshot info on every block dev in output of 'info
    snapshots'

 migration/savevm.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 95 insertions(+), 8 deletions(-)

-- 
2.8.1

[Qemu-devel] [PATCH 1/2 V4] hmp: use snapshot name to determine whether a snapshot is 'fully available'

[Lin Ma]

Currently qemu 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 uses '--' instead of snapshot id in output
because the snapshot id is not guaranteed to be the same on all images.
For instance:
(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

Signed-off-by: Lin Ma <lma@suse.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 migration/savevm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 38b85ee..a8f22da 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2230,7 +2230,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     available_snapshots = g_new0(int, nb_sns);
     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) {
             available_snapshots[total] = i;
             total++;
         }
@@ -2241,6 +2241,10 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "\n");
         for (i = 0; i < total; i++) {
             sn = &sn_tab[available_snapshots[i]];
+            /* The ID is not guaranteed to be the same on all images, so
+             * overwrite it.
+             */
+            pstrcpy(sn->id_str, sizeof(sn->id_str), "--");
             bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn);
             monitor_printf(mon, "\n");
         }
-- 
2.8.1

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

[Lin Ma]

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 makes 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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 migration/savevm.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 90 insertions(+), 7 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index a8f22da..95e9efa 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2200,12 +2200,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(, 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");
@@ -2222,25 +2241,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 *next_sn;
         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, next_sn) {
+                    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.
              */
@@ -2249,11 +2308,35 @@ 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,
+                       "\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");
+        }
+    }
+
+    QTAILQ_FOREACH(image_entry, &image_list, next) {
+        SnapshotEntry *next_sn;
+        QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next,
+                            next_sn) {
+            g_free(snapshot_entry);
+        }
+        g_free(image_entry);
+    }
     g_free(sn_tab);
-    g_free(available_snapshots);
+    g_free(global_snapshots);
 
 }
 
-- 
2.8.1

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

On 04.07.2016 11:31, 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 makes 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>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

I said it looked good apart from what I said, but I didn't give an R-b,
so this shouldn't be here.

> ---
>  migration/savevm.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a8f22da..95e9efa 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c

[...]

> @@ -2249,11 +2308,35 @@ 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,
> +                       "\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");
> +        }
> +    }
> +
> +    QTAILQ_FOREACH(image_entry, &image_list, next) {

This needs to be a QTAILQ_FOREACH_SAFE(), too.

Max

> +        SnapshotEntry *next_sn;
> +        QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next,
> +                            next_sn) {
> +            g_free(snapshot_entry);
> +        }
> +        g_free(image_entry);
> +    }
>      g_free(sn_tab);
> -    g_free(available_snapshots);
> +    g_free(global_snapshots);
>  
>  }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Lin Ma]

>>> Max Reitz <mreitz@redhat.com> 2016/7/6 星期三 上午 1:26 >>>
>On 04.07.2016 11:31, 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 makes 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>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
>I said it looked good apart from what I said, but I didn't give an R-b,
>so this shouldn't be here.
Got it.

>
>> ---
>>  migration/savevm.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 90 insertions(+), 7 deletions(-)
>> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index a8f22da..95e9efa 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>
>[...]
>
>> @@ -2249,11 +2308,35 @@ 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,
>> +					   "\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");
>> +	    }
>> +    }
>> +
>> +    QTAILQ_FOREACH(image_entry, &image_list, next) {
>
>This needs to be a QTAILQ_FOREACH_SAFE(), too.
>
>Max
OK, I'd like to define such a var: 'ImageEntry *next_ie' for this QTAILQ_FOREACH_SAFE,
put it above this line.
How about it?
 
Thanks.

>>
>> +	    SnapshotEntry *next_sn;
>> +	    QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snapshots, next,
>> +						    next_sn) {
>> +		    g_free(snapshot_entry);
>> +	    }
>> +	    g_free(image_entry);
>> +    }
>>	  g_free(sn_tab);
>> -    g_free(available_snapshots);
>> +    g_free(global_snapshots);
>>  
>>  }
>>  
>> 

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]

On 06.07.2016 07:42, Lin Ma wrote:
> 
> 
>>>> Max Reitz <mreitz@redhat.com> 2016/7/6 星期三 上午 1:26 >>>
>>On 04.07.2016 11:31, 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 makes 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>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>>I said it looked good apart from what I said, but I didn't give an R-b,
>>so this shouldn't be here.
> Got it.
> 
>>
>>> ---
>>>  migration/savevm.c | 97
> ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 90 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index a8f22da..95e9efa 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>
>>[...]
>>
>>> @@ -2249,11 +2308,35 @@ 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,
>>> +                       "\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");
>>> +        }
>>> +    }
>>> +
>>> +    QTAILQ_FOREACH(image_entry, &image_list, next) {
>>
>>This needs to be a QTAILQ_FOREACH_SAFE(), too.
>>
>>Max
> OK, I'd like to define such a var: 'ImageEntry *next_ie' for this
> QTAILQ_FOREACH_SAFE,
> put it above this line.
> How about it?

You should put the definition at the start of this function, since the
qemu coding style mandates variable declarations to only be at the start
of the block they are in.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]