From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37751) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VKP6X-0000at-8n for qemu-devel@nongnu.org; Fri, 13 Sep 2013 04:52:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VKP6K-0002XF-DK for qemu-devel@nongnu.org; Fri, 13 Sep 2013 04:51:53 -0400 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:44449 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VKP6K-0002X0-20 for qemu-devel@nongnu.org; Fri, 13 Sep 2013 04:51:40 -0400 Message-ID: <5232D218.9010402@kamp.de> Date: Fri, 13 Sep 2013 10:51:36 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1378481953-23099-1-git-send-email-stefanha@redhat.com> <1378481953-23099-31-git-send-email-stefanha@redhat.com> <5232C711.4050708@kamp.de> <5232CC44.9090009@redhat.com> In-Reply-To: <5232CC44.9090009@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 30/42] block: define get_block_status return value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Anthony Liguori , qemu-devel@nongnu.org, Stefan Hajnoczi On 13.09.2013 10:26, Paolo Bonzini wrote: > Il 13/09/2013 10:04, Peter Lieven ha scritto: >> On 06.09.2013 17:39, Stefan Hajnoczi wrote: >>> From: Paolo Bonzini >>> >>> Define the return value of get_block_status. Bits 0, 1, 2 and 9-62 >>> are valid; bit 63 (the sign bit) is reserved for errors. Bits 3-8 >>> are left for future extensions. >>> >>> The return code is compatible with the old is_allocated API: if a driver >>> only returns 0 or 1 (aka BDRV_BLOCK_DATA) like is_allocated used to, >>> clients of is_allocated will not have any change in behavior. Still, >>> we will return more precise information in the next patches and the >>> new definition of bdrv_is_allocated is already prepared for this. >>> >>> Reviewed-by: Eric Blake >>> Signed-off-by: Paolo Bonzini >>> Signed-off-by: Stefan Hajnoczi >>> --- >>> block.c | 10 ++++++++-- >>> include/block/block.h | 26 ++++++++++++++++++++++++++ >>> 2 files changed, 34 insertions(+), 2 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index a5bfe74..97406ec 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -3093,7 +3093,7 @@ static int64_t coroutine_fn >>> bdrv_co_get_block_status(BlockDriverState *bs, >>> if (!bs->drv->bdrv_co_get_block_status) { >>> *pnum = nb_sectors; >>> - return 1; >>> + return BDRV_BLOCK_DATA; >>> } >>> return bs->drv->bdrv_co_get_block_status(bs, sector_num, >>> nb_sectors, pnum); >>> @@ -3143,7 +3143,13 @@ int64_t bdrv_get_block_status(BlockDriverState >>> *bs, int64_t sector_num, >>> int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t >>> sector_num, >>> int nb_sectors, int *pnum) >>> { >>> - return bdrv_get_block_status(bs, sector_num, nb_sectors, pnum); >>> + int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, >>> pnum); >>> + if (ret < 0) { >> sorry if I repeat myself. >> >> isn't here a *pnum = 0 missing? > I think an earlier patch makes it unnecessary, because I'm checking the > return status of bdrv_get_block_status and bdrv_is_allocated. In any > case the right place to put it would be in bdrv_get_block_status. Ok, then I think for safety it should be added here: @@ -3104,13 +3151,14 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum); if (ret < 0) { + *pnum = 0; return ret; } because the new get_block_status api requires a ret < 0 if an error occurs. in case of an error someone will write sth like ret = -EINVAL in the block driver. this not necessary implies *pnum = 0; the old bdrv_is_allocated framework required a *pnum = 0 on error, but if someone (like me for iscsi) creates bdrv_get_block_status from scratch will not explicity do this. Peter