[Qemu-devel] Re: [PATCH 05/14] ide: Remove redundant IDEState member conf

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 28.05.2010 15:38, schrieb Markus Armbruster:
>> Commit 428c149b added IDEState member conf to let commit 0009baf1 find
>> the BlockConf from there.  It exists only for qdev drives, created via
>> ide_drive_initfn(), not for drives created via ide_init2().
>> 
>> But for a qdev drive, we can just as well reach its IDEDevice, which
>> contains the BlockConf.  Do that, and revert the parts of commit
>> 428c149b that add IDEState member conf.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/ide/core.c     |   16 +++++++---------
>>  hw/ide/internal.h |    4 +---
>>  hw/ide/qdev.c     |    3 +--
>>  3 files changed, 9 insertions(+), 14 deletions(-)
>> 
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 066fecb..c3334b1 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -98,6 +98,7 @@ static void ide_identify(IDEState *s)
>>  {
>>      uint16_t *p;
>>      unsigned int oldsize;
>> +    IDEDevice *dev;
>>  
>>      if (s->identify_set) {
>>      memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
>> @@ -165,8 +166,9 @@ static void ide_identify(IDEState *s)
>>      put_le16(p + 101, s->nb_sectors >> 16);
>>      put_le16(p + 102, s->nb_sectors >> 32);
>>      put_le16(p + 103, s->nb_sectors >> 48);
>> -    if (s->conf && s->conf->physical_block_size)
>> -        put_le16(p + 106, 0x6000 | get_physical_block_exp(s->conf));
>> +    dev = s->unit ? s->bus->slave : s->bus->master;
>> +    if (dev && dev->conf.physical_block_size)
>
> If this is meant to be the same condition it would be dev->conf && ...
>
> I can't really tell right now if it can happen that either dev or
> dev->conf can be NULL here. In a quick attempt I couldn't get either one.
>
> So, which version is correct, or should the first part be dropped
> completely?

dev->conf can't ever be null:

struct IDEDevice {
    DeviceState qdev;
    uint32_t unit;
    BlockConf conf;
    char *version;
    char *serial;
};

If s->bus was set up by ide_qdev_init(), then dev cannot be null.  I
*think* it can be null for non-qdev IDE.  Three places still use that.
Maybe it's time to accidentally break them and wait for the screams ;)