From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50833) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ULPBR-0001Mf-UB for qemu-devel@nongnu.org; Thu, 28 Mar 2013 22:36:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ULPBP-0001d0-OT for qemu-devel@nongnu.org; Thu, 28 Mar 2013 22:36:48 -0400 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:37162) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ULPBP-0001ci-4P for qemu-devel@nongnu.org; Thu, 28 Mar 2013 22:36:47 -0400 Received: from /spool/local by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 29 Mar 2013 12:28:14 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id 29C352CE804D for ; Fri, 29 Mar 2013 13:36:34 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2T2NKDS33488954 for ; Fri, 29 Mar 2013 13:23:21 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2T2aWSd015561 for ; Fri, 29 Mar 2013 13:36:32 +1100 Message-ID: <5154FE04.5020506@linux.vnet.ibm.com> Date: Fri, 29 Mar 2013 10:35:48 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1363961953-13561-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1363961953-13561-12-git-send-email-xiawenc@linux.vnet.ibm.com> <20130328095412.GA3077@dhcp-200-207.str.redhat.com> In-Reply-To: <20130328095412.GA3077@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V10 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, armbru@redhat.com 于 2013-3-28 17:54, Kevin Wolf 写道: > Am 22.03.2013 um 15:19 hat Wenchao Xia geschrieben: >> Now image info will be retrieved as an embbed json object inside >> BlockDeviceInfo, backing chain info and all related internal snapshot >> info can be got in the enhanced recursive structure of ImageInfo. >> >> Signed-off-by: Wenchao Xia >> --- >> block/qapi.c | 39 ++++++++++++++++++++++++++-- >> include/block/qapi.h | 4 ++- >> qapi-schema.json | 5 +++- >> qmp-commands.hx | 67 +++++++++++++++++++++++++++++++++++++++++++++++++- >> 4 files changed, 109 insertions(+), 6 deletions(-) >> >> diff --git a/block/qapi.c b/block/qapi.c >> index df73f5b..9051947 100644 >> --- a/block/qapi.c >> +++ b/block/qapi.c >> @@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs, >> return 0; >> } >> >> -BlockInfo *bdrv_query_info(BlockDriverState *bs) >> +/* return 0 on success, and @p_info will be set only on success. */ >> +int bdrv_query_info(BlockDriverState *bs, >> + BlockInfo **p_info, >> + Error **errp) >> { >> BlockInfo *info = g_malloc0(sizeof(*info)); >> + BlockDriverState *bs0; >> + ImageInfo **p_image_info; >> + int ret = 0; > > ret is never changed, so this function always returns 0. I would suggest > to drop ret and make the function return type void. > My bad, I forgot to set its value, the interface is intend to return negative error number and errp both on fail. >> info->device = g_strdup(bs->device_name); >> info->type = g_strdup("unknown"); >> info->locked = bdrv_dev_is_medium_locked(bs); >> @@ -256,8 +262,29 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs) >> info->inserted->iops_wr = >> bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE]; >> } >> + >> + bs0 = bs; >> + p_image_info = &info->inserted->image; >> + while (1) { >> + if (bdrv_query_image_info(bs0, p_image_info, errp)) { >> + goto err; >> + } Sorry ret is missing here, it should be: ret = bdrv_query_image_info(bs0, p_image_info, errp)); if (ret) { goto err; } I'll correct it. >> + if (bs0->drv && bs0->backing_hd) { >> + bs0 = bs0->backing_hd; >> + (*p_image_info)->has_backing_image = true; >> + p_image_info = &((*p_image_info)->backing_image); >> + } else { >> + break; >> + } >> + } >> } >> - return info; >> + >> + *p_info = info; >> + return 0; >> + >> + err: >> + qapi_free_BlockInfo(info); >> + return ret; >> } >> >> SnapshotInfoList *qmp_query_snapshots(Error **errp) >> @@ -284,11 +311,17 @@ BlockInfoList *qmp_query_block(Error **errp) >> >> while ((bs = bdrv_next(bs))) { >> BlockInfoList *info = g_malloc0(sizeof(*info)); >> - info->value = bdrv_query_info(bs); >> + if (bdrv_query_info(bs, &info->value, errp)) { >> + goto err; >> + } > > Consequently, you've got the error handling wrong here, the if condition > is never true. It should look more or less like this (the syntax details > may be wrong): > > Error *local_err; > bdrv_query_info(bs, &info->value, &local_err); > if (error_is_set(local_err)) { > error_propagate(err, local_err); > goto err; > } > > Kevin > -- Best Regards Wenchao Xia