From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39902) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNGBZ-0000Z3-K1 for qemu-devel@nongnu.org; Wed, 03 Apr 2013 01:24:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UNGBY-0004Ll-Eu for qemu-devel@nongnu.org; Wed, 03 Apr 2013 01:24:37 -0400 Received: from e28smtp07.in.ibm.com ([122.248.162.7]:35981) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNGBX-0004K1-JT for qemu-devel@nongnu.org; Wed, 03 Apr 2013 01:24:36 -0400 Received: from /spool/local by e28smtp07.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 3 Apr 2013 10:50:17 +0530 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 838433940053 for ; Wed, 3 Apr 2013 10:54:27 +0530 (IST) Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r335OOhU10093036 for ; Wed, 3 Apr 2013 10:54:24 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r335ORiM019323 for ; Wed, 3 Apr 2013 16:24:27 +1100 Message-ID: <515BBCF7.7050505@linux.vnet.ibm.com> Date: Wed, 03 Apr 2013 13:24:07 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1364903250-10429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1364903250-10429-6-git-send-email-xiawenc@linux.vnet.ibm.com> <515B832F.7050103@redhat.com> In-Reply-To: <515B832F.7050103@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.com, armbru@redhat.com, lcapitulino@redhat.com, pbonzini@redhat.com 于 2013-4-3 9:17, Eric Blake 写道: > On 04/02/2013 05:47 AM, Wenchao Xia wrote: >> This patch adds function bdrv_query_snapshot_info_list(), which will >> retrieve snapshot info of an image in qmp object format. The implementation >> is based on the code moved from qemu-img.c with modification to fit more >> for qmp based block layer API. >> >> Signed-off-by: Wenchao Xia >> --- >> block/qapi.c | 55 ++++++++++++++++++++++++++++++++++++++----------- >> include/block/qapi.h | 4 ++- >> qemu-img.c | 5 +++- >> 3 files changed, 49 insertions(+), 15 deletions(-) > >> +/* >> + * return 0 on success, @p_list will be set only on success, and caller need to > > s/need/needs/ > >> + * check *p_list on success. > > I wonder if this wording would be any better: > > Returns 0 on success, with *p_list either set to describe snapshot > information, or NULL because there are no snapshots. Returns -1 on > error, with *p_list untouched. > >> + */ >> +int bdrv_query_snapshot_info_list(BlockDriverState *bs, >> + SnapshotInfoList **p_list, >> + Error **errp) >> { > > At any rate, my only commentary was on grammar and a possible wording > for a comment, while the code itself is fine from my viewpoint; so feel > free to add: > > Reviewed-by: Eric Blake > >> +++ b/qemu-img.c >> @@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(const char *filename, >> >> info = g_new0(ImageInfo, 1); >> bdrv_collect_image_info(bs, info, filename); >> - bdrv_collect_snapshots(bs, info); >> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) && >> + info->snapshots) { >> + info->has_snapshots = true; >> + } > > Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is > untouched on error. Since you are ignoring any errors, you technically > could write: > > bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL); > if (info->snapshots) { > info->has_snapshots = true; > } > > for the same semantics. That means that as of this commit, no caller > cares about the return value of bdrv_query_snapshot_info_list (they only > care about whether info->snapshots was changed to non-null), so it could > return void for a slightly simpler implementation. > > But I don't know if any later patches in the series start to care about > which error was returned. > Yes it would be cared in ImageInfo retrieving later. This section in qemu-img.c would be replaced later so did not check strictly the returned value. -- Best Regards Wenchao Xia