Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize
Ekaterina Tumanova writes: > 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. Fair enough. bdrv_get_geometry() is a silly wrapper around bdrv_nb_sectors() that maps failure to zero sectors. I hope to kill it some time. Doesn't help you now. > I thought probe might be more suitable, since we do not "get" the value > for sure. maybe "detect"? Feel free to stick to "probe". >> 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. You'd have to do crazy stuff to defeat the static type checker. Please stick to the common technique. [...]
Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize
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_RDWR0x0002 #define BDRV_O_SNAPSHOT0x0008 /* 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.
Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize
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_RDWR0x0002 > #define BDRV_O_SNAPSHOT0x0008 /* 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 :)
Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize
Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova: > Add driver functions for geometry and blocksize detection > [...] > /* > * 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; > question for Kevin/Stefan/Marcus, is belows rc type of return code ok for you (see patch 5 for its usage)? > +struct ProbeBlockSize { > +int rc; > +struct BlockSize { > +uint16_t phys; > +uint16_t log; > +} size; > +}; Kate, can you make this a typedef so that you dont need to drag along "struct"? > + > +struct ProbeGeometry { > +int rc; > +struct HDGeometry { > +uint32_t heads; > +uint32_t sectors; > +uint32_t cylinders; > +uint32_t start; > +} geo; > +}; dito Otherwise looks sane.
[Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize
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; +} + +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; +}; + +struct ProbeGeometry { +int rc; +struct HDGeometry { +uint32_t heads; +uint32_t sectors; +uint32_t cylinders; +uint32_t start; +} geo; +}; + #define BDRV_O_RDWR0x0002 #define BDRV_O_SNAPSHOT0x0008 /* 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; }; -- 1.8.5.5