qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
@ 2020-03-23 15:17 Peter Maydell
  2020-03-23 15:33 ` Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Peter Maydell @ 2020-03-23 15:17 UTC (permalink / raw)
  To: qemu-devel; +Cc: John Snow, Philippe Mathieu-Daudé, qemu-ppc, qemu-block

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 <peter.maydell@linaro.org>
---
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));
 
         bmdma_init(&s->bus[i], &s->bmdma[i], s);
         s->bmdma[i].bus = &s->bus[i];
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
  2020-03-23 15:17 [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() Peter Maydell
@ 2020-03-23 15:33 ` Philippe Mathieu-Daudé
  2020-03-23 17:04 ` BALATON Zoltan
  2020-03-24 20:43 ` Mark Cave-Ayland
  2 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-23 15:33 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel; +Cc: John Snow, qemu-ppc, qemu-block

On 3/23/20 4:17 PM, 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 <peter.maydell@linaro.org>
> ---
> 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));
>   
>           bmdma_init(&s->bus[i], &s->bmdma[i], s);
>           s->bmdma[i].bus = &s->bus[i];
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
  2020-03-23 15:17 [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() Peter Maydell
  2020-03-23 15:33 ` Philippe Mathieu-Daudé
@ 2020-03-23 17:04 ` BALATON Zoltan
  2020-03-23 17:11   ` Peter Maydell
  2020-03-23 18:43   ` John Snow
  2020-03-24 20:43 ` Mark Cave-Ayland
  2 siblings, 2 replies; 8+ messages in thread
From: BALATON Zoltan @ 2020-03-23 17:04 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-ppc, Philippe Mathieu-Daudé, John Snow, qemu-devel, qemu-block

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 <peter.maydell@linaro.org>
> ---
> 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

>
>         bmdma_init(&s->bus[i], &s->bmdma[i], s);
>         s->bmdma[i].bus = &s->bus[i];
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
  2020-03-23 17:04 ` BALATON Zoltan
@ 2020-03-23 17:11   ` Peter Maydell
  2020-03-23 18:43   ` John Snow
  1 sibling, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2020-03-23 17:11 UTC (permalink / raw)
  To: BALATON Zoltan
  Cc: qemu-ppc, Philippe Mathieu-Daudé,
	John Snow, QEMU Developers, Qemu-block

On Mon, 23 Mar 2020 at 17:04, BALATON Zoltan <balaton@eik.bme.hu> 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 <peter.maydell@linaro.org>
> > ---
> > 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
  2020-03-23 17:04 ` BALATON Zoltan
  2020-03-23 17:11   ` Peter Maydell
@ 2020-03-23 18:43   ` John Snow
  2020-03-23 19:52     ` BALATON Zoltan
  1 sibling, 1 reply; 8+ messages in thread
From: John Snow @ 2020-03-23 18:43 UTC (permalink / raw)
  To: BALATON Zoltan, Peter Maydell
  Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel, qemu-block



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 <peter.maydell@linaro.org>
>> ---
>> 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 <jsnow@redhat.com>


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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
  2020-03-23 18:43   ` John Snow
@ 2020-03-23 19:52     ` BALATON Zoltan
  0 siblings, 0 replies; 8+ messages in thread
From: BALATON Zoltan @ 2020-03-23 19:52 UTC (permalink / raw)
  To: John Snow
  Cc: qemu-ppc, Peter Maydell, Philippe Mathieu-Daudé,
	qemu-devel, qemu-block

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Mon, 23 Mar 2020, John Snow wrote:
> 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 <peter.maydell@linaro.org>
>>> ---
>>> 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 <jsnow@redhat.com>
>
>
> The named temporary is fine. We probably should be using a named
> temporary in the other locations, too.

Yes that's fine too if using cast more than once is less efficient. Could 
you change the existing DEVICE(dev) also to ds when applying please? 
Probably no need for a v2 for that.

> 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.

I've tried that AmigaOS still boots on sam460ex so:

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

The sii3112 should also work on x86 or other platforms with Linux's 
sata_sil driver but only as a 2nd non-bootable controller because we don't 
have option ROM and BIOS probably has no driver. Apart from that it's a 
common PCI SATA controller so likely a lot of guests have driver for it.

Regards,
BALATON Zoltan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
  2020-03-23 15:17 [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() Peter Maydell
  2020-03-23 15:33 ` Philippe Mathieu-Daudé
  2020-03-23 17:04 ` BALATON Zoltan
@ 2020-03-24 20:43 ` Mark Cave-Ayland
  2020-03-24 21:05   ` John Snow
  2 siblings, 1 reply; 8+ messages in thread
From: Mark Cave-Ayland @ 2020-03-24 20:43 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel
  Cc: qemu-ppc, Philippe Mathieu-Daudé, John Snow, qemu-block

On 23/03/2020 15:17, 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 <peter.maydell@linaro.org>
> ---
> 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));
>  
>          bmdma_init(&s->bus[i], &s->bmdma[i], s);
>          s->bmdma[i].bus = &s->bus[i];

Looks like there is similar use of qemu_allocate_irqs() in via-ide and cmd646-ide,
and also reviewing my latest via-ide changes I spotted a silly mistake which was
obviously left in from a previous experimental version.

I'm not sure why Coverity doesn't pick up these other occurrences, however I'll send
along a patchset for this shortly.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
  2020-03-24 20:43 ` Mark Cave-Ayland
@ 2020-03-24 21:05   ` John Snow
  0 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2020-03-24 21:05 UTC (permalink / raw)
  To: Mark Cave-Ayland, Peter Maydell, qemu-devel
  Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-block



On 3/24/20 4:43 PM, Mark Cave-Ayland wrote:
> On 23/03/2020 15:17, 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 <peter.maydell@linaro.org>
>> ---
>> 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));
>>  
>>          bmdma_init(&s->bus[i], &s->bmdma[i], s);
>>          s->bmdma[i].bus = &s->bus[i];
> 
> Looks like there is similar use of qemu_allocate_irqs() in via-ide and cmd646-ide,
> and also reviewing my latest via-ide changes I spotted a silly mistake which was
> obviously left in from a previous experimental version.
> 
> I'm not sure why Coverity doesn't pick up these other occurrences, however I'll send
> along a patchset for this shortly.
> 

OK;

I will rescind my PR and will re-send it with your patches included.

--js



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-03-24 21:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-23 15:17 [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() Peter Maydell
2020-03-23 15:33 ` Philippe Mathieu-Daudé
2020-03-23 17:04 ` BALATON Zoltan
2020-03-23 17:11   ` Peter Maydell
2020-03-23 18:43   ` John Snow
2020-03-23 19:52     ` BALATON Zoltan
2020-03-24 20:43 ` Mark Cave-Ayland
2020-03-24 21:05   ` John Snow

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).