Re: [PATCH] ide: Get rid of IDEDrive struct

[John Snow <jsnow@redhat.com>]

On 8/5/20 4:41 PM, Peter Maydell wrote:
> On Wed, 5 Aug 2020 at 20:49, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>> The struct had a single field (IDEDevice dev), and is only used
>> in the QOM type declarations and property lists.  We can simply
>> use the IDEDevice struct directly instead.
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> @@ -327,7 +323,6 @@ static void ide_hd_class_init(ObjectClass *klass, void *data)
>>   static const TypeInfo ide_hd_info = {
>>       .name          = "ide-hd",
>>       .parent        = TYPE_IDE_DEVICE,
>> -    .instance_size = sizeof(IDEDrive),
>>       .class_init    = ide_hd_class_init,
>>   };
> 
> This is one of those areas where this change works and reduces
> amount of code, but on the other hand it means the QOM type
> doesn't follow the common pattern for a leaf type of:
>   * it has a struct
>   * it has cast macros that cast to that struct
>   * the typeinfo instance_size is the size of that struct
> (it wasn't exactly following this pattern before, of course).
> 
> We define in https://wiki.qemu.org/Documentation/QOMConventions
> (in the 'When to create class types and macros' bit at the bottom)
> what we expect for whether to provide class cast macros/a
> class struct/class_size in the TypeInfo, essentially recommending
> that types follow one of two patterns (simple leaf class with no
> methods or class members, vs everything else) even if in a
> particular case you could take a short-cut and not define
> everything. We haven't really defined similar "this is the
> standard pattern, provide it all even if you don't strictly
> need it" rules for the instance struct/macros. Maybe we should?
> 
> Just a thought, not a nak; I know we have quite a number
> of types that take this kind of "we don't really need to
> provide all the standard QOM macros/structs/etc" approach
> (some of which I wrote!).
> 

I'll defer to your judgment here. The IDE stuff is very confusing, but I 
don't know the best way to wrangle it to make it less confusing.

I assume at some point migration compatibility gets in the way of any 
REAL refactoring that might start to make this code make more sense.

Open to suggestions.

--js