Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

2014-11-28 Thread Markus Armbruster 
Ekaterina Tumanova tuman...@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 tuman...@linux.vnet.ibm.com writes:

 Add driver functions for geometry and blocksize detection

 Signed-off-by: Ekaterina Tumanova tuman...@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.

[...]

Re: [Qemu-devel] [PATCH v2 1/6] geometry: add bdrv functions for geometry and blocksize

2014-11-27 Thread Markus Armbruster 
I'm sorry for the delay.  I got the flu and have difficulties thinking
straight for longer than a few minutes.

Ekaterina Tumanova tuman...@linux.vnet.ibm.com writes:

 Add driver functions for geometry and blocksize detection

 Signed-off-by: Ekaterina Tumanova tuman...@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_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

2014-11-27 Thread Ekaterina Tumanova 

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 tuman...@linux.vnet.ibm.com writes:


Add driver functions for geometry and blocksize detection

Signed-off-by: Ekaterina Tumanova tuman...@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_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

2014-11-21 Thread Christian Borntraeger 
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.