From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UEBLU-0005Ny-I5 for qemu-devel@nongnu.org; Fri, 08 Mar 2013 23:25:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UEBLP-0007yS-Dy for qemu-devel@nongnu.org; Fri, 08 Mar 2013 23:25:20 -0500 Received: from e28smtp04.in.ibm.com ([122.248.162.4]:57294) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UEBLO-0007wF-TI for qemu-devel@nongnu.org; Fri, 08 Mar 2013 23:25:15 -0500 Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 9 Mar 2013 09:52:11 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 23353394002D for ; Sat, 9 Mar 2013 09:55:09 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r294P1LD24510610 for ; Sat, 9 Mar 2013 09:55:04 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r294P6A5007969 for ; Sat, 9 Mar 2013 15:25:06 +1100 Message-ID: <513AB99B.4090502@linux.vnet.ibm.com> Date: Sat, 09 Mar 2013 12:24:59 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1362636445-7188-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1362636445-7188-9-git-send-email-xiawenc@linux.vnet.ibm.com> <513A6C55.2080507@redhat.com> In-Reply-To: <513A6C55.2080507@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, armbru@redhat.com, pbonzini@redhat.com 于 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 >> --- >> 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