From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu1ZU-0008O0-Ac for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:05:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xu1ZL-0006ke-Vn for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:05:32 -0500 Received: from e06smtp13.uk.ibm.com ([195.75.94.109]:42996) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu1ZL-0006kN-Mk for qemu-devel@nongnu.org; Thu, 27 Nov 2014 11:05:23 -0500 Received: from /spool/local by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 27 Nov 2014 16:05:22 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 5CD5B2190041 for ; Thu, 27 Nov 2014 16:04:51 +0000 (GMT) Received: from d06av06.portsmouth.uk.ibm.com (d06av06.portsmouth.uk.ibm.com [9.149.37.217]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sARG5I1r18743508 for ; Thu, 27 Nov 2014 16:05:18 GMT Received: from d06av06.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sARB2J9I029829 for ; Thu, 27 Nov 2014 06:02:20 -0500 Message-ID: <54774BBD.8020506@linux.vnet.ibm.com> Date: Thu, 27 Nov 2014 19:05:17 +0300 From: Ekaterina Tumanova MIME-Version: 1.0 References: <1416392276-10408-1-git-send-email-tumanova@linux.vnet.ibm.com> <1416392276-10408-2-git-send-email-tumanova@linux.vnet.ibm.com> <87d288g0ap.fsf@blackfin.pond.sub.org> In-Reply-To: <87d288g0ap.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, dahi@linux.vnet.ibm.com, Public KVM Mailing List , mihajlov@linux.vnet.ibm.com, borntraeger@de.ibm.com, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com On 11/27/2014 05:55 PM, Markus Armbruster wrote: > I'm sorry for the delay. I got the flu and have difficulties thinking > straight for longer than a few minutes. > > Ekaterina Tumanova writes: > >> Add driver functions for geometry and blocksize detection >> >> Signed-off-by: Ekaterina Tumanova >> --- >> block.c | 26 ++++++++++++++++++++++++++ >> include/block/block.h | 20 ++++++++++++++++++++ >> include/block/block_int.h | 3 +++ >> 3 files changed, 49 insertions(+) >> >> diff --git a/block.c b/block.c >> index a612594..5df35cf 100644 >> --- a/block.c >> +++ b/block.c >> @@ -548,6 +548,32 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp) >> } >> } >> >> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs) >> +{ >> + BlockDriver *drv = bs->drv; >> + struct ProbeBlockSize err_geo = { .rc = -1 }; >> + >> + assert(drv != NULL); >> + if (drv->bdrv_probe_blocksizes) { >> + return drv->bdrv_probe_blocksizes(bs); >> + } >> + >> + return err_geo; >> +} > > I'm not sure about "probe". Naming is hard. "get"? > There was already "bdrv_get_geometry", and I wanted the _geometry and _blocksize functions to use the same naming convention. I thought probe might be more suitable, since we do not "get" the value for sure. maybe "detect"? > Squashing status and actual payload into a single struct to use as > return type isn't wrong, but unusual. When the payload can't represent > failure conveniently, we usually return status, and write the payload to > a buffer provided by the caller, like this: > > int bdrv_get_blocksizes(BlockDriverState *bs, > uint16_t *physical_blk_sz, > uint16_t *logical_blk_sz) > > Or, with a struct to hold both sizes: > > int bdrv_get_blocksizes(BlockDriverState *bs, BlockSizes *bsz) > Do you insist on changing that? Returning a struct via stack seemed useful to me, since there was less probability of caller allocating a buffer of incorrect size or smth like that. > Such a struct should ideally be used in other places where we store both > sizes. > > A brief function contract comment wouldn't hurt. Something like > > /* > * Try to get @bs's logical and physical block size. > * Block sizes are always a multiple of BDRV_SECTOR_SIZE. > * On success, store them in ... and return 0. > * On failure, return -errno. > */ > will do >> + >> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs) >> +{ >> + BlockDriver *drv = bs->drv; >> + struct ProbeGeometry err_geo = { .rc = -1 }; >> + >> + assert(drv != NULL); >> + if (drv->bdrv_probe_geometry) { >> + return drv->bdrv_probe_geometry(bs); >> + } >> + >> + return err_geo; >> +} >> + >> /* >> * Create a uniquely-named empty temporary file. >> * Return 0 upon success, otherwise a negative errno value. >> diff --git a/include/block/block.h b/include/block/block.h >> index 5450610..3287dbc 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -60,6 +60,24 @@ typedef enum { >> BDRV_REQ_MAY_UNMAP = 0x4, >> } BdrvRequestFlags; >> >> +struct ProbeBlockSize { >> + int rc; >> + struct BlockSize { >> + uint16_t phys; >> + uint16_t log; >> + } size; >> +}; > > Use of uint16_t for block sizes is silly, but not your fault, you just > follow existing usage. > >> + >> +struct ProbeGeometry { >> + int rc; >> + struct HDGeometry { >> + uint32_t heads; >> + uint32_t sectors; >> + uint32_t cylinders; >> + uint32_t start; >> + } geo; >> +}; >> + >> #define BDRV_O_RDWR 0x0002 >> #define BDRV_O_SNAPSHOT 0x0008 /* open the file read only and save writes in a snapshot */ >> #define BDRV_O_TEMPORARY 0x0010 /* delete the file after use */ >> @@ -538,6 +556,8 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs); >> * the old #AioContext is not executing. >> */ >> void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context); >> +struct ProbeBlockSize bdrv_probe_blocksizes(BlockDriverState *bs); >> +struct ProbeGeometry bdrv_probe_geometry(BlockDriverState *bs); >> >> void bdrv_io_plug(BlockDriverState *bs); >> void bdrv_io_unplug(BlockDriverState *bs); >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index a1c17b9..830e564 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -271,6 +271,9 @@ struct BlockDriver { >> void (*bdrv_io_unplug)(BlockDriverState *bs); >> void (*bdrv_flush_io_queue)(BlockDriverState *bs); >> >> + struct ProbeBlockSize (*bdrv_probe_blocksizes)(BlockDriverState *bs); >> + struct ProbeGeometry (*bdrv_probe_geometry)(BlockDriverState *bs); >> + >> QLIST_ENTRY(BlockDriver) list; >> }; > > Callbacks need contracts even more than functions do. I know this file > is full of bad examples. Let's not add more :) > Thanks! Kate.