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

[Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>]

On 02/13/2015 03:23 PM, Christian Borntraeger wrote:
> 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?
>

yes.
will be fix in v7.