Patchwork [V6,11/14] qmp: add interface query-snapshots

login
register
mail settings
Submitter Wayne Xia
Date Feb. 18, 2013, 2:09 p.m.
Message ID <1361196578-19016-12-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/221429/
State New
Headers show

Comments

Wayne Xia - Feb. 18, 2013, 2:09 p.m.
This interface now return valid internal snapshots for whole vm
or a single block device.
  Note that filter use bdrv_can_read_snapshot() instead of
bdrv_can_snapshot(), which should be the correct behavior in information
retrieving funtion.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c          |   44 ++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   19 +++++++++++++++++++
 qmp-commands.hx  |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 0 deletions(-)
Wayne Xia - Feb. 18, 2013, 10:46 p.m.
Hi, Eric
   About the interface,there is actually requirement to know internal
snapshots in an image of a backing file, so I think the API should be
improved as:

# @query-snapshots:
#
# Get a list of internal snapshots for whole virtual machine or a single
# block device. Note that in first case, only valid internal snapshot
# will be returned, inconsistent ones will be ignored.
#
# @device: #optional the name of the device to get snapshot info from. # 
          If not specified, only valid snapshots for whole vm would be
#          returned.
# @image: #optional the image's name in the backing chain, only valid
#          when device is specified. If it is not specified, the
#          internal snapshots on the top of the chain will be shown.
#          Otherwise qemu will try search the image on the chain on
#          that device.
#
# Returns: a list of @SnapshotInfo describing all consistent virtual # 
         machine
#          snapshots.
#
# Since: 1.5
##
{ 'command': 'query-snapshots',
   'data': { '*device': 'str', '*image': 'str' },
   'returns': ['SnapshotInfo'] }

   What do you think of the API?
>
>   ##
> +# @query-snapshots:
> +#
> +# Get a list of internal snapshots for whole virtual machine or a single
> +# block device. Note that in first case, only valid internal snapshot will be
> +# returned, inconsistent ones will be ignored.
> +#
> +# @device: #optional the name of the device to get snapshot info from. If not
> +#          specified, only valid snapshots for whole vm would be returned.
> +#
> +# Returns: a list of @SnapshotInfo describing all consistent virtual machine
> +#          snapshots.
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'query-snapshots',
> +  'data': { '*device': 'str' },
> +  'returns': ['SnapshotInfo'] }
> +
> +##
>   # @BlockDeviceStats:
>   #
>   # Statistics of a virtual block device or a block backing device.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 292d61e..846e23e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1819,6 +1819,59 @@ EQMP
>       },
>
Eric Blake - Feb. 20, 2013, 10:57 p.m.
On 02/18/2013 03:46 PM, Wenchao Xia wrote:
> Hi, Eric
>   About the interface,there is actually requirement to know internal
> snapshots in an image of a backing file, so I think the API should be
> improved as:
> 
> # @query-snapshots:
> #
> # Get a list of internal snapshots for whole virtual machine or a single
> # block device. Note that in first case, only valid internal snapshot
> # will be returned, inconsistent ones will be ignored.
> #
> # @device: #optional the name of the device to get snapshot info from. #
>          If not specified, only valid snapshots for whole vm would be
> #          returned.
> # @image: #optional the image's name in the backing chain, only valid
> #          when device is specified. If it is not specified, the
> #          internal snapshots on the top of the chain will be shown.
> #          Otherwise qemu will try search the image on the chain on
> #          that device.

Why not just always show all information for all images in the chain?
Is it an efficiency issue, where allowing the user to limit information
up front will result in a more responsive command?  If that is not a
concern, then making the command simpler by having just a @device
argument, and no @image argument, seems like the better way to go.

But I definitely agree that if you have:

base.qcow2 <- top.qcow2

and both base.qcow2 and top.qcow2 each contain internal snapshots, that
there should be a way to get at all of that information, and not be
limited to just the information on the internal snapshots in top.qcow2.
Eric Blake - Feb. 20, 2013, 11:30 p.m.
On 02/20/2013 03:57 PM, Eric Blake wrote:
> On 02/18/2013 03:46 PM, Wenchao Xia wrote:
>> Hi, Eric
>>   About the interface,there is actually requirement to know internal
>> snapshots in an image of a backing file, so I think the API should be
>> improved as:
>>
>> # @query-snapshots:
>> #
>> # Get a list of internal snapshots for whole virtual machine or a single
>> # block device. Note that in first case, only valid internal snapshot
>> # will be returned, inconsistent ones will be ignored.
>> #
>> # @device: #optional the name of the device to get snapshot info from. #
>>          If not specified, only valid snapshots for whole vm would be
>> #          returned.
>> # @image: #optional the image's name in the backing chain, only valid
>> #          when device is specified. If it is not specified, the
>> #          internal snapshots on the top of the chain will be shown.
>> #          Otherwise qemu will try search the image on the chain on
>> #          that device.
> 
> Why not just always show all information for all images in the chain?
> Is it an efficiency issue, where allowing the user to limit information
> up front will result in a more responsive command?  If that is not a
> concern, then making the command simpler by having just a @device
> argument, and no @image argument, seems like the better way to go.
> 
> But I definitely agree that if you have:
> 
> base.qcow2 <- top.qcow2
> 
> and both base.qcow2 and top.qcow2 each contain internal snapshots, that
> there should be a way to get at all of that information, and not be
> limited to just the information on the internal snapshots in top.qcow2.

Thinking more about it, a consistent snapshot is one that we can revert
to right now, without having to do any block device munging.  So if
@device is not supplied, we are limited to JUST snapshots at the top of
any image chain (any internal snapshots embedded in a backing file can
only be reached for purposes of a loadvm if we also rearrange the block
device to point to the backing file).  But the moment you are interested
in looking at the snapshot information stored in a single device,
without regards to the rest of the system, then knowing everything about
the chain makes sense.  But looking at your example:


+-> { "execute": "query-snapshots" }
+<- {
+      "return":[
+         {
+            "id": "1",
+            "name": "snapshot1",
+            "vm-state-size": 0,
+            "date-sec": 10000200,
+            "date-nsec": 12,
+            "vm-clock-sec": 206,
+            "vm-clock-nsec": 30
+         },
+         {
+            "id": "2",
+            "name": "snapshot2",
+            "vm-state-size": 34000000,
+            "date-sec": 13000200,
+            "date-nsec": 32,
+            "vm-clock-sec": 406,
+            "vm-clock-nsec": 31
+         }
+      ]
+   }

that does not give us a way to see which image in a backing chain owns
which internal snapshot names.

I think the best plan of attack is to not try and confuse the two tasks.
 Looking at earlier in your series, I think that 'query-images' is the
best place to return information about the entire backing chain AND all
internal snapshots at each point in the backing chain.

That is, leave 'query-snapshots' for JUST obtaining a list of snapshots
that 'loadvm' would work on (only consistent snapshots, present in only
the top image, and no optional @device parameter), and make
'query-images' be the work-horse for determining all details about a
backing chain, something like:

+-> { "execute": "query-images" }
+<- {
+      "return":[
+         {
+            "device":"ide0-hd0",
+            "image":{
+               "filename":"disks/test0.img",
+               "format":"qcow2",
+               "virtual-size":1024000
+            }
+         },
+         {
+            "device":"ide0-hd1",
+            "image":{
+               "filename":"disks/test1.img",
+               "format":"qcow2",
+               "virtual-size":2048000,
+               "snapshots":[
+                  {
+                     "id": "1",
+                     "name": "snapshot1",
+                     "vm-state-size": 0,
+                     "date-sec": 10000200,
+                     "date-nsec": 12,
+                     "vm-clock-sec": 206,
+                     "vm-clock-nsec": 30
+                  }
+               ],
+               "backing":{
+                  "filename":"disks/test1base.img",
+                  "format":"qcow2",
+                  "virtual-size":2048000,
+                  "snapshots":[
+                     {
+                         "id":"2", ...
+                     }
+                  ]
+               }
+            }
+         }
+      ]
+   }
Wayne Xia - Feb. 26, 2013, 7:35 a.m.
于 2013-2-21 7:30, Eric Blake 写道:
> On 02/20/2013 03:57 PM, Eric Blake wrote:
>> On 02/18/2013 03:46 PM, Wenchao Xia wrote:
>>> Hi, Eric
>>>    About the interface,there is actually requirement to know internal
>>> snapshots in an image of a backing file, so I think the API should be
>>> improved as:
>>>
>>> # @query-snapshots:
>>> #
>>> # Get a list of internal snapshots for whole virtual machine or a single
>>> # block device. Note that in first case, only valid internal snapshot
>>> # will be returned, inconsistent ones will be ignored.
>>> #
>>> # @device: #optional the name of the device to get snapshot info from. #
>>>           If not specified, only valid snapshots for whole vm would be
>>> #          returned.
>>> # @image: #optional the image's name in the backing chain, only valid
>>> #          when device is specified. If it is not specified, the
>>> #          internal snapshots on the top of the chain will be shown.
>>> #          Otherwise qemu will try search the image on the chain on
>>> #          that device.
>>
>> Why not just always show all information for all images in the chain?
>> Is it an efficiency issue, where allowing the user to limit information
>> up front will result in a more responsive command?  If that is not a
>> concern, then making the command simpler by having just a @device
>> argument, and no @image argument, seems like the better way to go.
>>
>> But I definitely agree that if you have:
>>
>> base.qcow2 <- top.qcow2
>>
>> and both base.qcow2 and top.qcow2 each contain internal snapshots, that
>> there should be a way to get at all of that information, and not be
>> limited to just the information on the internal snapshots in top.qcow2.
>
> Thinking more about it, a consistent snapshot is one that we can revert
> to right now, without having to do any block device munging.  So if
> @device is not supplied, we are limited to JUST snapshots at the top of
> any image chain (any internal snapshots embedded in a backing file can
> only be reached for purposes of a loadvm if we also rearrange the block
> device to point to the backing file).  But the moment you are interested
> in looking at the snapshot information stored in a single device,
> without regards to the rest of the system, then knowing everything about
> the chain makes sense.  But looking at your example:
>
>
> +-> { "execute": "query-snapshots" }
> +<- {
> +      "return":[
> +         {
> +            "id": "1",
> +            "name": "snapshot1",
> +            "vm-state-size": 0,
> +            "date-sec": 10000200,
> +            "date-nsec": 12,
> +            "vm-clock-sec": 206,
> +            "vm-clock-nsec": 30
> +         },
> +         {
> +            "id": "2",
> +            "name": "snapshot2",
> +            "vm-state-size": 34000000,
> +            "date-sec": 13000200,
> +            "date-nsec": 32,
> +            "vm-clock-sec": 406,
> +            "vm-clock-nsec": 31
> +         }
> +      ]
> +   }
>
> that does not give us a way to see which image in a backing chain owns
> which internal snapshot names.
>
> I think the best plan of attack is to not try and confuse the two tasks.
>   Looking at earlier in your series, I think that 'query-images' is the
> best place to return information about the entire backing chain AND all
> internal snapshots at each point in the backing chain.
>
> That is, leave 'query-snapshots' for JUST obtaining a list of snapshots
> that 'loadvm' would work on (only consistent snapshots, present in only
> the top image, and no optional @device parameter), and make
> 'query-images' be the work-horse for determining all details about a
> backing chain, something like:
>
> +-> { "execute": "query-images" }
> +<- {
> +      "return":[
> +         {
> +            "device":"ide0-hd0",
> +            "image":{
> +               "filename":"disks/test0.img",
> +               "format":"qcow2",
> +               "virtual-size":1024000
> +            }
> +         },
> +         {
> +            "device":"ide0-hd1",
> +            "image":{
> +               "filename":"disks/test1.img",
> +               "format":"qcow2",
> +               "virtual-size":2048000,
> +               "snapshots":[
> +                  {
> +                     "id": "1",
> +                     "name": "snapshot1",
> +                     "vm-state-size": 0,
> +                     "date-sec": 10000200,
> +                     "date-nsec": 12,
> +                     "vm-clock-sec": 206,
> +                     "vm-clock-nsec": 30
> +                  }
> +               ],
> +               "backing":{
> +                  "filename":"disks/test1base.img",
> +                  "format":"qcow2",
> +                  "virtual-size":2048000,
> +                  "snapshots":[
> +                     {
> +                         "id":"2", ...
> +                     }
> +                  ]
> +               }
> +            }
> +         }
> +      ]
> +   }
>
>

   I agree with you about the interface, will use query-images for
device's inside snapshot.

Patch

diff --git a/block.c b/block.c
index 28afaae..b407296 100644
--- a/block.c
+++ b/block.c
@@ -2885,6 +2885,50 @@  SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
     return head;
 }
 
+/* check if sn exist on all block devices, 0 means valid */
+static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, void *opaque)
+{
+    BlockDriverState *bs = (BlockDriverState *)opaque, *bs1 = NULL;
+    QEMUSnapshotInfo s, *sn_info = &s;
+    int ret = 0;
+
+    while ((bs1 = bdrv_next(bs1))) {
+        if (bdrv_can_read_snapshot(bs1) && bs1 != bs) {
+            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
+            if (ret < 0) {
+                ret = -1;
+                break;
+            }
+        }
+    }
+    return ret;
+}
+
+SnapshotInfoList *qmp_query_snapshots(bool has_device,
+                                      const char *device,
+                                      Error **errp)
+{
+    BlockDriverState *bs;
+
+    if (has_device) {
+        /* internal snapshots for single device */
+        bs = bdrv_find(device);
+        if (!bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+            return NULL;
+        }
+        return bdrv_query_snapshot_infolist(bs, NULL, bs, errp);
+    }
+
+    /* internal snapshot for whole vm */
+    bs = bdrv_snapshots();
+    if (!bs) {
+        error_setg(errp, "No available block device supports snapshots\n");
+        return NULL;
+    }
+    return bdrv_query_snapshot_infolist(bs, snapshot_filter_vm, bs, errp);
+}
+
 /* collect all internal snapshot info in a image for ImageInfo */
 static void collect_snapshots_info(BlockDriverState *bs,
                                    ImageInfo *info,
diff --git a/qapi-schema.json b/qapi-schema.json
index 70777c0..c0ff2c5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -824,6 +824,25 @@ 
   'returns': ['DeviceImageInfo'] }
 
 ##
+# @query-snapshots:
+#
+# Get a list of internal snapshots for whole virtual machine or a single
+# block device. Note that in first case, only valid internal snapshot will be
+# returned, inconsistent ones will be ignored.
+#
+# @device: #optional the name of the device to get snapshot info from. If not
+#          specified, only valid snapshots for whole vm would be returned.
+#
+# Returns: a list of @SnapshotInfo describing all consistent virtual machine
+#          snapshots.
+#
+# Since: 1.5
+##
+{ 'command': 'query-snapshots',
+  'data': { '*device': 'str' },
+  'returns': ['SnapshotInfo'] }
+
+##
 # @BlockDeviceStats:
 #
 # Statistics of a virtual block device or a block backing device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 292d61e..846e23e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1819,6 +1819,59 @@  EQMP
     },
 
 SQMP
+query-snapshots
+-----------
+
+Show the internal consistent snapshot information.
+
+Each snapshot information is stored in a json-object and the returned value
+is a json-array of all snapshots.
+
+Each json-object contain the following:
+
+- "id": unique snapshot id (json-string)
+- "name": internal snapshot name (json-string)
+- "vm-state-size": size of the VM state in bytes (json-int)
+- "date-sec": UTC date of the snapshot in seconds (json-int)
+- "date-nsec": fractional part in nano seconds to be used with date-sec(json-int)
+- "vm-clock-sec": VM clock relative to boot in seconds (json-int)
+- "vm-clock-nsec": fractional part in nano seconds to be used with vm-clock-sec (json-int)
+
+Example:
+
+-> { "execute": "query-snapshots" }
+<- {
+      "return":[
+         {
+            "id": "1",
+            "name": "snapshot1",
+            "vm-state-size": 0,
+            "date-sec": 10000200,
+            "date-nsec": 12,
+            "vm-clock-sec": 206,
+            "vm-clock-nsec": 30
+         },
+         {
+            "id": "2",
+            "name": "snapshot2",
+            "vm-state-size": 34000000,
+            "date-sec": 13000200,
+            "date-nsec": 32,
+            "vm-clock-sec": 406,
+            "vm-clock-nsec": 31
+         }
+      ]
+   }
+
+EQMP
+
+    {
+        .name       = "query-snapshots",
+        .args_type  = "device:B?",
+        .mhandler.cmd_new = qmp_marshal_input_query_snapshots,
+    },
+
+SQMP
 query-blockstats
 ----------------