Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[John Snow]

On 3/23/20 1:04 PM, BALATON Zoltan wrote:
> On Mon, 23 Mar 2020, Peter Maydell wrote:
>> Coverity points out (CID 1421984) that we are leaking the
>> memory returned by qemu_allocate_irqs(). We can avoid this
>> leak by switching to using qdev_init_gpio_in(); the base
>> class finalize will free the irqs that this allocates under
>> the hood.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>> This is how the 'use qdev gpio' approach to fixing the leak looks.
>> Disclaimer: I have only tested this with "make check", nothing more.
>>
>> hw/ide/sii3112.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 06605d7af2b..2ae6f5d9df6 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev,
>> Error **errp)
>> {
>>     SiI3112PCIState *d = SII3112_PCI(dev);
>>     PCIIDEState *s = PCI_IDE(dev);
>> +    DeviceState *ds = DEVICE(dev);
>>     MemoryRegion *mr;
>> -    qemu_irq *irq;
>>     int i;
>>
>>     pci_config_set_interrupt_pin(dev->config, 1);
>> @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev,
>> Error **errp)
>>     memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio,
>> 0, 16);
>>     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>
>> -    irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
>> +    qdev_init_gpio_in(ds, sii3112_set_irq, 2);
>>     for (i = 0; i < 2; i++) {
>>         ide_bus_new(&s->bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
>> -        ide_init2(&s->bus[i], irq[i]);
>> +        ide_init2(&s->bus[i], qdev_get_gpio_in(ds, i));
> 
> Maybe we could just use DEVICE(dev) here and above as well just like in
> ide_bus_new above just to keep it consistent and avoid the confusion
> caused by having dev, d, s and now also ds. DEVICE(dev) is short and
> clear enough I think.
> 
> Regards,
> BALATON Zoltan
> 

Reviewed-by: John Snow <address@hidden>


The named temporary is fine. We probably should be using a named
temporary in the other locations, too.

I will run my usual tests, but admit I don't really test the non-x86
boards directly. Do you want to give a tested-by on this, if it matters
to you? Otherwise, I'm fairly content to trust Peter's judgment here.

--js