On Tue, 28 Jul 2020 at 09:48, Thomas Huth <[email protected]> wrote: > > On 27/07/2020 16.41, Peter Maydell wrote: > > On Mon, 27 Jul 2020 at 14:03, Keqian Zhu <[email protected]> wrote: > >> > >> Avoid covering object refcount of qemu_irq, otherwise it may causes > >> memory leak. > >> > >> Signed-off-by: Keqian Zhu <[email protected]> > >> --- > >> hw/core/irq.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/core/irq.c b/hw/core/irq.c > >> index fb3045b912..59af4dfc74 100644 > >> --- a/hw/core/irq.c > >> +++ b/hw/core/irq.c > >> @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, > >> qemu_irq_handler handler, int n) > >> int i; > >> qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); > >> for (i = 0; i < n; i++) { > >> - *old_irqs[i] = *gpio_in[i]; > >> + old_irqs[i]->handler = gpio_in[i]->handler; > >> + old_irqs[i]->opaque = gpio_in[i]->opaque; > >> + > >> gpio_in[i]->handler = handler; > >> gpio_in[i]->opaque = &old_irqs[i]; > >> } > > > > This function is leaky by design, because it doesn't do anything > > with the old_irqs array and there's no function for un-intercepting > > the IRQs (which would need to free that memory). This is not ideal > > but OK because it's only used in the test suite. > > I think this could better be done without calling qemu_allocate_irqs(): > Simply call qemu_allocate_irq() (without "s" at the end) within the > for-loop for each irq instead. What do you think?
Well, what are we trying to do with the function? I think that your suggestion still doesn't really fix the leak in the sense that there's no mechanism for undoing the operation and freeing the memory allocated by qemu_allocate_irq(). The whole concept of the function is pretty dubious because it's arbitrarily re-plugging IRQs, which in the whole of the rest of QEMU are assumed to be something that's wired up by board code or by an SoC device that "owns" the two devices it's connecting. That's fine for test-case purposes, and for the qtest code that is the only in-tree user of this function we could construct a (nominally) leak-free version of the function by having it return some kind of handle that could be passed to a qemu_irq_remove_intercept_in() to clean up [this would fix a Coverity nag, which would be the main benefit...]. I agree that copying the refcount here looks wrong, but in what situation is the refcount on any of the qemu_irqs involved changing anyway? For non-testing uses of this function it gets trickier (what if the device on one end or the other is destroyed while the intercept is in place, for instance?) So I'm a bit sceptical of ideas about extending the uses of this function to non-test-code, and I think I'd want to see what the intended non-test uses were... thanks -- PMM
