| Message ID | 1361196578-19016-12-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
> },
>
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.
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", ... + } + ] + } + } + } + ] + }
δΊ 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.
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 ----------------
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(-)