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

[Corey Minyard <minyard@acm.org>]

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, minyard@acm.org 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