From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50665) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu0UR-0003dz-GU for qemu-devel@nongnu.org; Thu, 27 Nov 2014 09:56:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xu0UL-0007WM-BC for qemu-devel@nongnu.org; Thu, 27 Nov 2014 09:56:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51931) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu0UL-0007Vm-41 for qemu-devel@nongnu.org; Thu, 27 Nov 2014 09:56:09 -0500 From: Markus Armbruster References: <1416392276-10408-1-git-send-email-tumanova@linux.vnet.ibm.com> <1416392276-10408-2-git-send-email-tumanova@linux.vnet.ibm.com> Date: Thu, 27 Nov 2014 15:55:58 +0100 In-Reply-To: <1416392276-10408-2-git-send-email-tumanova@linux.vnet.ibm.com> (Ekaterina Tumanova's message of "Wed, 19 Nov 2014 11:17:51 +0100") Message-ID: <87d288g0ap.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Ekaterina Tumanova Cc: kwolf@redhat.com, borntraeger@de.ibm.com, Public KVM Mailing List , mihajlov@linux.vnet.ibm.com, dahi@linux.vnet.ibm.com, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com 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"? 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) 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. */ > + > +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 :)