[PATCH] ide: Get rid of IDEDrive struct

[PATCH] ide: Get rid of IDEDrive struct

[Eduardo Habkost]

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>
---
 hw/ide/qdev.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 27ff1f7f66..dd3867d8b3 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -157,10 +157,6 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
 
 /* --------------------------------- */
 
-typedef struct IDEDrive {
-    IDEDevice dev;
-} IDEDrive;
-
 static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
@@ -297,19 +293,19 @@ static void ide_drive_realize(IDEDevice *dev, Error **errp)
 }
 
 #define DEFINE_IDE_DEV_PROPERTIES()                     \
-    DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
-    DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
-    DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
-    DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
-    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
-    DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+    DEFINE_BLOCK_PROPERTIES(IDEDevice, conf),        \
+    DEFINE_BLOCK_ERROR_PROPERTIES(IDEDevice, conf),  \
+    DEFINE_PROP_STRING("ver",  IDEDevice, version),  \
+    DEFINE_PROP_UINT64("wwn",  IDEDevice, wwn, 0),   \
+    DEFINE_PROP_STRING("serial",  IDEDevice, serial),\
+    DEFINE_PROP_STRING("model", IDEDevice, model)
 
 static Property ide_hd_properties[] = {
     DEFINE_IDE_DEV_PROPERTIES(),
-    DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(IDEDevice, conf),
     DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
-                IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
-    DEFINE_PROP_UINT16("rotation_rate", IDEDrive, dev.rotation_rate, 0),
+                IDEDevice, chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+    DEFINE_PROP_UINT16("rotation_rate", IDEDevice, rotation_rate, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -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,
 };
 
@@ -350,7 +345,6 @@ static void ide_cd_class_init(ObjectClass *klass, void *data)
 static const TypeInfo ide_cd_info = {
     .name          = "ide-cd",
     .parent        = TYPE_IDE_DEVICE,
-    .instance_size = sizeof(IDEDrive),
     .class_init    = ide_cd_class_init,
 };
 
@@ -373,7 +367,6 @@ static void ide_drive_class_init(ObjectClass *klass, void *data)
 static const TypeInfo ide_drive_info = {
     .name          = "ide-drive",
     .parent        = TYPE_IDE_DEVICE,
-    .instance_size = sizeof(IDEDrive),
     .class_init    = ide_drive_class_init,
 };
 
-- 
2.26.2

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

[Peter Maydell]

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

thanks
-- PMM

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

[Eduardo Habkost]

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.

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.

> 
> 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.

> 
> 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
> 

-- 
Eduardo

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
>> 

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

[Daniel P. Berrangé]

On Thu, Aug 06, 2020 at 07:58:06AM +0200, Markus Armbruster wrote:
> 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.

I think following a consistent pattern everywhere is important,
because people look at existing code to guide their new code.
The boilerplate pain is very real, but I think my patch series
you point to will reduce the burden sufficiently that the kind
of optimization proposed here is not required.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Peter Maydell]

On Wed, 5 Aug 2020 at 23:14, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Wed, Aug 05, 2020 at 09:41:25PM +0100, Peter Maydell wrote:
> > 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.

Most leaf types need this. Consider a simple device type
like TYPE_CMSDK_APB_UART. It has a TYPE_* name so that
users of it can instantiate it; it has a CMSDKAPBUART struct
that holds all the device state; it has the CMSDK_APB_UART()
cast macro so that code that gets a Device* or Object* can
get at the struct. Leaf types like ide-hd which have no
actual state of their own are I think the less common case:
most leaf types do have at least some member variables.

As Markus says, we can have a couple of standard patterns
if we want to (as we do for the class-macro conventions);
I just wanted to explain that lots of leaf types work the
way I outline above.

thanks
-- PMM

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

[John Snow]

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