Re: [Qemu-devel] [PATCH 22/22] core/sysbus: remove the SysBusDeviceClass::initpath

[Eduardo Habkost]

On Fri, Nov 23, 2018 at 11:10:40AM +0800, maozy wrote:
> Hi, Eduardo
> 
> On 11/20/18 7:31 AM, Eduardo Habkost wrote:
> > On Mon, Nov 19, 2018 at 08:08:20PM +0800, Mao Zhongyi wrote:
> > > Currently, all sysbus devices have been converted to realize(),
> > > so remove this path.
> > > 
> > > Cc: address@hidden
> > > Cc: address@hidden
> > > Cc: address@hidden
> > > Cc: address@hidden
> > > Cc: address@hidden
> > > Cc: address@hidden
> > > Cc: address@hidden
> > > 
> > > Signed-off-by: Mao Zhongyi <address@hidden>
> > > Signed-off-by: Zhang Shengju <address@hidden>
> > > ---
> > >   hw/core/sysbus.c    | 15 ---------------
> > >   include/hw/sysbus.h |  3 ---
> > >   2 files changed, 18 deletions(-)
> > > 
> > > diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> > > index 7ac36ad3e7..030ad426c1 100644
> > > --- a/hw/core/sysbus.c
> > > +++ b/hw/core/sysbus.c
> > > @@ -201,20 +201,6 @@ void sysbus_init_ioports(SysBusDevice *dev, uint32_t 
> > > ioport, uint32_t size)
> > >       }
> > >   }
> > > -/* TODO remove once all sysbus devices have been converted to realize */
> > > -static void sysbus_realize(DeviceState *dev, Error **errp)
> > > -{
> > > -    SysBusDevice *sd = SYS_BUS_DEVICE(dev);
> > > -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
> > > -
> > > -    if (!sbc->init) {
> > > -        return;
> > > -    }
> > > -    if (sbc->init(sd) < 0) {
> > > -        error_setg(errp, "Device initialization failed");
> > > -    }
> > > -}
> > 
> > Nice.  :)
> > 
> > 
> > > -
> > >   DeviceState *sysbus_create_varargs(const char *name,
> > >                                      hwaddr addr, ...)
> > >   {
> > > @@ -327,7 +313,6 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
> > >   static void sysbus_device_class_init(ObjectClass *klass, void *data)
> > >   {
> > >       DeviceClass *k = DEVICE_CLASS(klass);
> > > -    k->realize = sysbus_realize;
> > 
> > Have you ensured this won't break any subclasses that
> > saved the original realize function on a parent_realize field?
> 
> Thanks for the catch.
> 
> > Now they will have parent_realize set to NULL.
> 
> In order to void the subclasses whose parent_realize field is
> set to NULL, the k->realize function must be retained even
> though it doesn't do anything practical. Just like this:
> 
> 
> -/* TODO remove once all sysbus devices have been converted to realize*/
>  static void sysbus_realize(DeviceState *dev, Error **errp)
>  {
> -    SysBusDevice *sd = SYS_BUS_DEVICE(dev);
> -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
> -
> -    if (!sbc->init) {
> -        return;
> -    }
> -    if (sbc->init(sd) < 0) {
> -        error_setg(errp, "Device initialization failed");
> -    }
>  }
> 
> it doesn't look elegant, but I didn't think of a better way, if you
> can give me some hints, I really appreciate it. :)

I think this is good enough for now (as long as there's a comment
like Peter suggested).  Allowing parent_realize to be NULL would
be inconvenient to all code that uses parent_realize today.

Personally, I would love to get rid of parent_realize entirely.
We could simply provide a helper to let device subclasses call
the parent's realize function without the need to copy function
pointers around.

-- 
Eduardo