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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Aug 05, 2020 at 09:41:25PM +0100, 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).
>
> Is this really a pattern that exists and we want to follow?
> I don't see why that pattern would be useful for simple leaf
> types.

I think the pattern exists, but we deviate from it in quite a few
places, probably just because it's so much boilerplate.

Related: Daniel's "[PATCH 0/4] qom: reduce boilerplate required for
declaring and defining objects".  Perhaps Daniel has an opinion on
taking shortcuts with leaf types.

> Also, in this case the code wasn't even following that pattern:
> it was using the same IDEDrive struct for all TYPE_IDE_DEVICE
> subtypes.

Rule of thumb: hw/ide/ is a bad example.  I don't mean to belittle the
efforts of quite a few people over the years.  It used to be worse.

>> 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?
>
> I think we should include the instance struct/macros in the
> recommendations there, but I would expect those recommendations
> to apply only to non-leaf types.

I'm fine with having a separate convention for leaf types if that helps,
but please let's have a convention.  I like my QOM boilerplate
uncreative.

>> 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!).
>> 
>> thanks
>> -- PMM
>>