δΊ 2013-3-9 6:55, Eric Blake ει:
On 03/06/2013 11:07 PM, Wenchao Xia wrote:
This patch adds a parameter to tell whether return valid snapshots
for whole VM only.
Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com>
---
block/qapi.c | 39 +++++++++++++++++++++++++++++++++++++--
include/block/qapi.h | 1 +
qemu-img.c | 3 ++-
3 files changed, 40 insertions(+), 3 deletions(-)
+ * @sn: snapshot info need to be checked.
s/need //
OK.
+ * return 0 if valid, it means load_vmstate() for it could succeed.
Reads awkwardly; how about:
it means load_vmstate() could load that snapshot.
I forgot it may not have vmstate() but only only block snapshot,
It should be:
return 0 if valid, it means the snapshot is consistent for the VM.
+ */
+static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
+{
+ BlockDriverState *bs1 = NULL;
+ QEMUSnapshotInfo s, *sn_info = &s;
+ int ret = 0;
+
+ /* Check logic is connected with load_vmstate():
+ Only check the devices that can snapshot, other devices that can't
+ take snapshot, for example, readonly ones, will be ignored in
+ load_vmstate(). */
+ while ((bs1 = bdrv_next(bs1))) {
+ if (bdrv_can_snapshot(bs1) && bs1 != bs) {
+ ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
+ if (ret < 0) {
+ ret = -1;
This function only returns 0 or -1...
OK, it should be bool.
+/* return 0 on success, and @p_list will be set only on success. If
+ @vm_snapshot is true, only the snapshot valid for whole vm will be got. */
grammar
If @vm_snapshot is true, limit the results to snapshots valid for the
whole vm.
Looks better, thanks.
-
+ if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {
...yet you are only ever calling it in a boolean context. Would it be
smarter to have the function return bool (true for valid vm snapshot,
false if the image snapshot is not usable as a vm snapshot)?
Also, it might be nicer to name it snapshot_valid_for_vm, as in:
if (vm_snapshot && !snapshot_valid_for_vm(...)) {
continue;
}
OK.
--
Best Regards
Wenchao Xia
