Re: [Qemu-devel] [PATCH v6 5/5] BlockConf: Call backend functions to detect geometry and blocksizes

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 19.01.2015 um 15:35 schrieb Ekaterina Tumanova:
> geometry: hd_geometry_guess function autodetects the drive geometry.
> This patch adds a block backend call, that probes the backing device
> geometry. If the inner driver method is implemented and succeeds
> (currently only for DASDs), the blkconf_geometry will pass-through
> the backing device geometry. Otherwise will fallback to old logic.
> 
> blocksize: This patch initializes blocksize properties to 0.
> In order to set the properly a blkconf_blocksizes was introduced.
> If user didn't set physical or logical blocksize, it will
> retrieve its value from a driver (which will return a default 512 for
> any backend but DASD).
> 
> The blkconf_blocksizes call was added to all users of BlkConf.
> 
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/block/block.c             | 20 ++++++++++++++++++++
>  hw/block/hd-geometry.c       | 10 +++++++++-
>  hw/block/nvme.c              |  1 +
>  hw/block/virtio-blk.c        |  1 +
>  hw/core/qdev-properties.c    |  3 ++-
>  hw/ide/qdev.c                |  1 +
>  hw/scsi/scsi-disk.c          |  1 +
>  hw/usb/dev-storage.c         |  1 +
>  include/hw/block/block.h     |  5 +++--
>  include/hw/qdev-properties.h |  4 ++--
>  10 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/block/block.c b/hw/block/block.c
> index a625773..09dd5f1 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -25,6 +25,26 @@ void blkconf_serial(BlockConf *conf, char **serial)
>      }
>  }
> 
> +void blkconf_blocksizes(BlockConf *conf)
> +{
> +    BlockBackend *blk = conf->blk;
> +    BlockSizes blocksizes;
> +    int backend_ret;
> +
> +    backend_ret = blk_probe_blocksizes(blk, &blocksizes);
> +    /* fill in detected values if they are not defined via qemu command line */
> +    if (!conf->physical_block_size && !backend_ret) {
> +        conf->physical_block_size = blocksizes.phys;
> +    } else {
> +        conf->physical_block_size = BDRV_SECTOR_SIZE;
> +    }
> +    if (!conf->logical_block_size && !backend_ret) {
> +        conf->logical_block_size = blocksizes.log;
> +    } else {
> +        conf->logical_block_size = BDRV_SECTOR_SIZE;
> +    }

When we are going to fix this, I found another bug:

This code will fail when logical_block_size and physical_block_size are given at the command line AND detection (backend_ret != 0) did not work. It will use BDRV_SECTOR_SIZE instead of the command line value.
With something like 

--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -33,15 +33,19 @@ void blkconf_blocksizes(BlockConf *conf)
 
     backend_ret = blk_probe_blocksizes(blk, &blocksizes);
     /* fill in detected values if they are not defined via qemu command line */
-    if (!conf->physical_block_size && !backend_ret) {
-        conf->physical_block_size = blocksizes.phys;
-    } else {
-        conf->physical_block_size = BDRV_SECTOR_SIZE;
+    if (!conf->physical_block_size) {
+        if (!backend_ret) {
+            conf->physical_block_size = blocksizes.phys;
+        } else {
+            conf->physical_block_size = BDRV_SECTOR_SIZE;
+        }
     }
-    if (!conf->logical_block_size && !backend_ret) {
-        conf->logical_block_size = blocksizes.log;
-    } else {
-        conf->logical_block_size = BDRV_SECTOR_SIZE;
+    if (!conf->logical_block_size) {
+	if (!backend_ret) {
+	        conf->logical_block_size = blocksizes.log;
+	} else {
+		conf->logical_block_size = BDRV_SECTOR_SIZE;
+	}
     }
 }
 

No?