Message ID | 1416392276-10408-2-git-send-email-tumanova@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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.
I'm sorry for the delay. I got the flu and have difficulties thinking straight for longer than a few minutes. Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > Add driver functions for geometry and blocksize detection > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> > --- > 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 :)
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 <tumanova@linux.vnet.ibm.com> writes: > >> Add driver functions for geometry and blocksize detection >> >> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> >> --- >> 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.
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> 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 <tumanova@linux.vnet.ibm.com> writes: >> >>> Add driver functions for geometry and blocksize detection >>> >>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> >>> --- >>> 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. [...]
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_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; };
Add driver functions for geometry and blocksize detection Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> --- block.c | 26 ++++++++++++++++++++++++++ include/block/block.h | 20 ++++++++++++++++++++ include/block/block_int.h | 3 +++ 3 files changed, 49 insertions(+)