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

[Paolo Bonzini]

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.

> 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