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
