Re: [Qemu-devel] [PATCH v3 0/4] Plug some memory leaks on unrealize

[Corey Minyard]

On 07/23/2016 10:16 AM, Paolo Bonzini wrote:
> On 23/07/2016 15:18, Corey Minyard wrote:
> > On 07/23/2016 02:46 AM, Paolo Bonzini wrote:
> > > On 22/07/2016 21:50, address@hidden wrote:
> > > > This has kind of opened a can of worms for me, though.  Looking
> > > > at a lot of the devices, there is no unrealize function and that
> > > > can leave a lot of things hanging.  And for ISA bus devices, there
> > > > is no way to unregister ports.
> > > Right, this is because they aren't hotpluggable.
> > >
> > > I should dig out the huge patchset I had to make timers statically
> > > allocated...
> > >
> > > Paolo
> > Am I correct in saying, then, that instead of adding a finalize
> > function to the IPMI BMC, we should instead make it not hot
> > pluggable?  And then the rest of my patches are not really
> > relevant.  I already have a function to set hotpluggable to
> > false for the BMCs, I can post that.
> If they are ISA devices they should already not be hot-unpluggable,
> because none of the ISA bridges implements HotplugHandler.  Because
> that's just the way the bus works, it shouldn't be an issue.

It's not exactly an ISA device.  This is a BMC that an ISA device
hooks to, but it's a separate device.

> >  From what I have seen, you can unrealize devices using the
> > API, even if they are not hot pluggable, by setting the realized
> > bool.  Is that ok?
> It's not great, but it's not a big deal either.
>
> The original idea behind "realize" was to have it as a sort of Vcc pin
> where a false/true pulse would work as a reset, but this never
> materialized.  Now the true->false transition on realize is really only
> used as part of a full guest-triggered hot-unplug sequence, which is
> guest->hotplug_handler_unplug->(method call)->object_unparent.
>
> Because all HotplugHandlers call object_unparent, which in turn ends up
> freeing the object, a false->true->false transition on realized (and
> thus the timer leak) is not guest-triggerable.
>
> There are various fixes, including:
>
> - making the device non-hotpluggable
>
> - moving the timer_new and timer_free respectively to instance_init and
> instance_finalize
>
> - making the timer static, which requires some small changes in the
> timer API.  Most of the last bullet is scriptable with Coccinelle.
>
> Right now I'd just do #2 or don't bother.
>
> Paolo
I think I'm going to opt for #1, because the device isn't hot
pluggable and if you try to unplug it qemu will crash.

-corey