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

[Eduardo Habkost]

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, address@hidden wrote:
> > > From: Marc-André Lureau <address@hidden>
> > > 
> > > 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 <address@hidden>
> > 
> > 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.

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?

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
further and remove the local 'gsi' variable and replace it with
'pcms->gsi' everywhere.

-- 
Eduardo