From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39071) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLeTv-00025N-VC for qemu-devel@nongnu.org; Thu, 06 Mar 2014 15:01:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLeTp-0004GP-TM for qemu-devel@nongnu.org; Thu, 06 Mar 2014 15:01:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:16330) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLeTp-0004G8-La for qemu-devel@nongnu.org; Thu, 06 Mar 2014 15:01:21 -0500 Message-ID: <5318D405.4080404@redhat.com> Date: Thu, 06 Mar 2014 21:01:09 +0100 From: Max Reitz MIME-Version: 1.0 References: <1393860533-2063-1-git-send-email-mreitz@redhat.com> <1393860533-2063-6-git-send-email-mreitz@redhat.com> <20140305161150.GG1709@irqsave.net> <5317849B.7030107@redhat.com> <20140305204122.GB4514@irqsave.net> <53178CC9.3070700@redhat.com> <20140305232210.GC5450@irqsave.net> In-Reply-To: <20140305232210.GC5450@irqsave.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 06.03.2014 00:22, Beno=EEt Canet wrote: > The Wednesday 05 Mar 2014 =E0 21:44:57 (+0100), Max Reitz wrote : >> On 05.03.2014 21:41, Beno=EEt Canet wrote: >>> The Wednesday 05 Mar 2014 =E0 21:10:03 (+0100), Max Reitz wrote : >>>> On 05.03.2014 17:11, Beno=EEt Canet wrote: >>>>> The Monday 03 Mar 2014 =E0 16:28:48 (+0100), Max Reitz wrote : >>>>>> Implement this function in the same way as raw_bsd does: Acknowled= ge >>>>>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET= _VALID >>>>>> and BDRV_BLOCK_DATA and derive the offset directly from the sector >>>>>> index) and add BDRV_BLOCK_RAW to the returned value. >>>>>> >>>>>> Signed-off-by: Max Reitz >>>>>> --- >>>>>> block/json.c | 9 +++++++++ >>>>>> 1 file changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/block/json.c b/block/json.c >>>>>> index a2f4691..7392802 100644 >>>>>> --- a/block/json.c >>>>>> +++ b/block/json.c >>>>>> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(= BlockDriverState *bs, >>>>>> return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flag= s); >>>>>> } >>>>>> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverS= tate *bs, >>>>>> + int64_t sect= or_num, >>>>>> + int nb_secto= rs, int *pnum) >>>>>> +{ >>>>>> + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_= DATA | >>>>>> + (sector_num << BDRV_SECTOR_BITS); >>>>>> +} >>>>> I don't understand what is the selling point of this method instead= of calling >>>>> bdrv_co_get_block_status on bs->file. >>>>> Some information risk to be lost and it does look like magic. >>>> This is the same what "raw" does. It just is more meaningful: This >>>> way, this function does not pretend to provide the blocks itself but >>>> instead tells the truth; that is, the blocks are provided by an >>>> underlying BDS (bs->file). >>>> >>>> I wasn't really sure what to do myself. Generally, this driver is >>>> actually meant to pretend that it provides the blocks itself. On the >>>> other hand, I tried to imitate the behavior or "raw", since this is >>>> something I can hope to be approximately correct. Also, as I've said >>>> before, the value returned here is in fact at least technically >>>> correct. >>> The raw_bsd driver have an additional *pnum =3D nb_sectors; at the be= gining of the >>> function. Did you left it on purpose ? >> Oops, no, I did not. Okay, if I even fail at copying code, the >> argument about "raw" being at least probably correct isn't worth >> anything here, I guess. ;-) > In fact I wonder i posix_raw act this way because one of the BDRV it do= es wrap > have simply no clue on how to answer to this request. If you take a look at bdrv_co_get_block_status() in block.c, you'll see=20 that BDRV_BLOCK_RAW actually results in a recursive call to=20 bdrv_get_block_status(). Max > Best regards > > Beno=EEt > >> Max >> >>> Best regards >>> >>> Beno=EEt >>>> Max >>>> >>>>> Best regards >>>>> >>>>> Beno=EEt >>>>> >>>>>> + >>>>>> static void json_invalidate_cache(BlockDriverState *bs) >>>>>> { >>>>>> return bdrv_invalidate_cache(bs->file); >>>>>> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json =3D { >>>>>> .bdrv_aio_discard =3D json_aio_discard, >>>>>> .bdrv_co_write_zeroes =3D json_co_write_zeroes, >>>>>> + .bdrv_co_get_block_status =3D json_co_get_block_status, >>>>>> .bdrv_invalidate_cache =3D json_invalidate_cache, >>>>>> --=20 >>>>>> 1.9.0 >>>>>> >>>>>>