| Message ID | 20160719085432.4572-25-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > --- > 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; > > /*< public >*/ > > -- > 2.9.0 > >
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()? > > > --- > > 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; > > > > /*< public >*/ > > > > -- > > 2.9.0 > > > > > > -- > Eduardo >
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. 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.
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.
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.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) 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.
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; /*< public >*/