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

[Peter Maydell]

On Mon, 23 Mar 2020 at 17:04, BALATON Zoltan <address@hidden> 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.

Usual style in these methods is to have local variables if
it's going to be used more than once or twice -- QOM casts
aren't free the way C casts are. We already do that here for
the SII3112_PCI() and the PCI_IDE().

In this case things are a bit confused by the function having
used "dev" for the PCIDevice* and 's' for the PCIIDEState.
QEMU is far from consistent in this matter, but usually 's'
is the pointer to the device's own state (ie SiI3112PCIState*
in this case) and the DeviceState* is 'dev'. The PCIDevice *
is often 'pci_dev'. I would call the PCIIDEState* 'pci_ide'.

I hadn't noticed the use of DEVICE() in ide_bus_new(); you're
right that we should be consistent about whether we use the cast
macro or the local variable.

thanks
-- PMM