Re: [Qemu-devel] [PATCH v2 23/30] bus: do not unref hotplug handler

[Marc-André Lureau]

Hi

On Wed, Feb 22, 2017 at 3:39 PM Paolo Bonzini <address@hidden> wrote:

>
>
> On 21/02/2017 15:14, Marc-André Lureau wrote:
> > Apparently, none of the bus owner give a reference to the hotplug
> > handler property, do not unref it on bus release.
> >
> > Furthermore, a bus is allowed to be its own hotplug handler, which can
> > be seen in qbus_set_bus_hotplug_handler() function. However, in this
> > case, the reference can't be given to the property, or this will create
> > a cyclic dependency and the bus will never be free.
> >
> > Each bus owner should manage the lifecycle of the hotplug handler.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
>
> Almost all qbus_set_hotplug_handler callers are using it to set the
> parent device (that is, the "adapter" or "bridge", whatever you want to
> call it) as the hotplug handler.
>
> The exception is piix4_update_bus_hotplug.  This one is the only case
> where OBJ_PROP_LINK_UNREF_ON_RELEASE would be right.  Luckily, there
> _is_ a reference to that device somewhere else to keep it alive, namely
> in /machine's acpi-device prop.  So this case is a bit hacky (not your
> fault) but works as well.  In addition the PIIX4_PM device is not
> hot(un)pluggable.
>

I understand you agree with my change, correct?

>
> Can you please add a comment to piix4_update_bus_hotplug explaining that
> the PIIX4PMState cannot outlive the PCIBus, because /machine keeps it
> alive?
>

 Do you mean PCIBus cannot outlive PIIX4PMState? (ie the PIIX4PMState will
always be there since it's linked from /machine)

thanks

>
> >
> > diff --git a/hw/core/bus.c b/hw/core/bus.c
> > index cf383fc1af..4651f24486 100644
> > --- a/hw/core/bus.c
> > +++ b/hw/core/bus.c
> > @@ -197,7 +197,7 @@ static void qbus_initfn(Object *obj)
> >                               TYPE_HOTPLUG_HANDLER,
> >                               (Object **)&bus->hotplug_handler,
> >                               object_property_allow_set_link,
> > -                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
> > +                             0,
> >                               NULL);
> >      object_property_add_bool(obj, "realized",
> >                               bus_get_realized, bus_set_realized, NULL);
> > --
> > 2.11.0.295.gd7dffce1c.dirty
> >
> >
> >
>
> --
Marc-André Lureau