|
From: | Wenchao Xia |
Subject: | Re: [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list() |
Date: | Sat, 09 Mar 2013 12:24:59 +0800 |
User-agent: | Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130215 Thunderbird/17.0.3 |
δΊ 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 <address@hidden> --- 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
[Prev in Thread] | Current Thread | [Next in Thread] |