From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQ83S-0006ng-Cf for qemu-devel@nongnu.org; Wed, 10 Apr 2013 23:20:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UQ83R-000874-1O for qemu-devel@nongnu.org; Wed, 10 Apr 2013 23:20:06 -0400 Received: from e28smtp08.in.ibm.com ([122.248.162.8]:53507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UQ83Q-000827-9U for qemu-devel@nongnu.org; Wed, 10 Apr 2013 23:20:04 -0400 Received: from /spool/local by e28smtp08.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Apr 2013 08:44:31 +0530 Received: from d28relay02.in.ibm.com (d28relay02.in.ibm.com [9.184.220.59]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id E5C0A394002D for ; Thu, 11 Apr 2013 08:49:59 +0530 (IST) Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay02.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3B3Ju9I46334174 for ; Thu, 11 Apr 2013 08:49:56 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3B3JxxT019657 for ; Thu, 11 Apr 2013 03:19:59 GMT Message-ID: <51662BCD.5070207@linux.vnet.ibm.com> Date: Thu, 11 Apr 2013 11:19:41 +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> <87wqsaxvwx.fsf@blackfin.pond.sub.org> In-Reply-To: <87wqsaxvwx.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=GB2312 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: Markus Armbruster Cc: kwolf@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com ÓÚ 2013-4-10 23:11, Markus Armbruster дµÀ: > Eric Blake writes: > >> 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. > > It actually returns -errno then, doesn't it? > Yes, I forgot change the comments in the version changing, thank you for the carefully review. >> >>> + */ >>> +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. > > Return the list, or NULL. > >> But I don't know if any later patches in the series start to care about >> which error was returned. > > Me neither :) > -- Best Regards Wenchao Xia