Re: [Qemu-devel] [PATCH 24/37] pc: keep gsi reference

["Marc-André Lureau" <mlureau@redhat.com>]

Hi

----- Original Message -----
> On Thu, Jul 21, 2016 at 01:27:35PM -0400, Marc-André Lureau wrote:
> > Hi
> > 
> > ----- Original Message -----
> > > On Tue, Jul 19, 2016 at 12:54:19PM +0400, marcandre.lureau@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > 
> > > > Further cleanup would need to call qemu_free_irq() at the appropriate
> > > > time, but for now this silences ASAN about direct leaks.
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > Is there a way to make ASAN happy without having to add a field
> > > to MachineState that we're not going to use for anything?
> > 
> > Well, the plan is rather to release it when no longer needed.
> > Would it be fine to call qemu_free_irqs() in
> > machine_finalize()?
> 
> It would be fine, I guess, but it looks pointless if we have lots
> of other resources allocated during PC machine initialization
> that are never released.

The main point, right now, is to have no direct leaks when running ASAN or valgrind, as they hide new introduced leaks that may be much worse. (it would also be good if we had no indirect leaks either, as this may also grow over time)

> But, see additional comment below:
> 
> > 
> > > 
> > > > ---
> > > >  hw/i386/pc_piix.c   | 1 +
> > > >  hw/i386/pc_q35.c    | 1 +
> > > >  include/hw/boards.h | 1 +
> > > >  3 files changed, 3 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > index a07dc81..b2db274 100644
> > > > --- a/hw/i386/pc_piix.c
> > > > +++ b/hw/i386/pc_piix.c
> > > > @@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
> > > >      } else {
> > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > >          GSI_NUM_PINS);
> > > >      }
> > > > +    machine->gsi = gsi;
> > > >  
> > > >      if (pcmc->pci_enabled) {
> > > >          pci_bus = i440fx_init(host_type,
> > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > index c5e8367..5dfb14f 100644
> > > > --- a/hw/i386/pc_q35.c
> > > > +++ b/hw/i386/pc_q35.c
> > > > @@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
> > > >      } else {
> > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > >          GSI_NUM_PINS);
> > > >      }
> > > > +    machine->gsi = gsi;
> > > >  
> > > >      /* create pci host bus */
> > > >      q35_host = Q35_HOST_DEVICE(qdev_create(NULL,
> > > >      TYPE_Q35_HOST_DEVICE));
> > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > index e46a744..289ba52 100644
> > > > --- a/include/hw/boards.h
> > > > +++ b/include/hw/boards.h
> > > > @@ -139,6 +139,7 @@ struct MachineState {
> > > >      /*< private >*/
> > > >      Object parent_obj;
> > > >      Notifier sysbus_notifier;
> > > > +    qemu_irq *gsi;
> 
> If this is used only by PC, doesn't it belong to PCMachineState?

right, i'll try to put it there

> Anyway, the new field would be very useful to help reduce the
> number of parameters of PC initialization functions (by making
> them just get a PCMachineState* argument). I would go even

Which functions do you have in mind?

> further and remove the local 'gsi' variable and replace it with
> 'pcms->gsi' everywhere.

ok, why not.