Re: [Qemu-devel] [PATCH v6 0/5] Geometry and blocksize detection for backing devices.

[Christian Borntraeger]

Am 13.02.2015 um 08:50 schrieb Kevin Wolf:
> Am 12.02.2015 um 17:42 hat Christian Borntraeger geschrieben:
>> Am 12.02.2015 um 16:46 schrieb Stefan Hajnoczi:
>>> On Mon, Jan 19, 2015 at 03:34:56PM +0100, Ekaterina Tumanova wrote:
>>>> Updates v5 -> v6:
>>>>
>>>> Minor Updates according the last review from Stefan Hajnoczi:
>>>> 1. Do not change the flow of code, factored out of raw_probe_alignment.
>>>> 2. added #ifdef __linux__ in 2 places of raw-posix.c, mentioned by 
>>>> reviewer.
>>>> 3. adjusted the comment hdev_probe_geometry according suggestment.
>>>> 4. use bdrv_nb_sectors(bs) instead of bs->total_sectors.
>>>> 5. do not discard error blk_probe_blocksizes(). now has rc.
>>>> 6. put the 512-byte default blocksize value in blkconf_blocksizes.
>>>> 7. drop the default parameter from the DEFINE_PROP_BLOCKSIZE() macro.
>>>
>>> Unfortunately this series breaks "make check" so it cannot be merged:
>>>
>>> GTESTER check-qtest-x86_64
>>> qemu-system-x86_64: logical_block_size must be 512 for IDE
>>> qemu-system-x86_64: Device initialization failed.
>>> qemu-system-x86_64: Initialization of device ide-cd failed
>>> Broken pipe
>>> GTester: last random seed: R02S942fac7e56eff09e8ab7a7f7fecf847e
>>>
>>
>> This particular message came in with 
>>
>> commit d20051856cd2fa8f10fed2d2a0b2751de5f7b20d
>> Author: Kevin Wolf <address@hidden>
>> Date:   Wed Dec 3 13:21:32 2014 +0100
>>
>>     ide: Check validity of logical block size
>>
>> so something like
>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> index 353854c..2680275 100644
>> --- a/hw/ide/qdev.c
>> +++ b/hw/ide/qdev.c
>> @@ -163,7 +163,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind 
>> kind)
>>          return -1;
>>      }
>>  
>> -    if (dev->conf.logical_block_size != 512) {
>> +    if (dev->conf.logical_block_size != 512 &&
>> +        dev->conf.logical_block_size != 0) {
>>          error_report("logical_block_size must be 512 for IDE");
>>          return -1;
>>      }
>>
>>
>> will fix this.
> 
> It would probably be better to set the default first and then make sure
> that the final value, no matter whether explicitly specified or default,
> is 512.
> 
> Kevin

+1 
Yes, this should minimize the impact.