On Thu, Jul 21, 2016 at 02:28:33PM -0400, Marc-André Lureau wrote:
> 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.lur...@redhat.com
> > > > wrote:
> > > > > From: Marc-André Lureau <marcandre.lur...@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.lur...@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)
I see. And you don't need to release it on finalize to reach that
goal, right? In this case I don't think we need the extra work.
(In case my previous message was unclear, I believe the field
will be useful, even if we don't release anything on finalize.)
>
> > 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?
i440fx_init(), pc_basic_device_init(), maybe others.
>
> > further and remove the local 'gsi' variable and replace it with
> > 'pcms->gsi' everywhere.
>
> ok, why not.
--
Eduardo
