From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43207) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhaUB-0000TD-PX for qemu-devel@nongnu.org; Thu, 01 Oct 2015 05:49:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhaU8-0002cn-Dt for qemu-devel@nongnu.org; Thu, 01 Oct 2015 05:49:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52752) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhaU8-0002cF-6Y for qemu-devel@nongnu.org; Thu, 01 Oct 2015 05:49:08 -0400 References: <1443689999-12182-1-git-send-email-armbru@redhat.com> <1443689999-12182-3-git-send-email-armbru@redhat.com> From: Thomas Huth Message-ID: <560D018F.8050801@redhat.com> Date: Thu, 1 Oct 2015 11:49:03 +0200 MIME-Version: 1.0 In-Reply-To: <1443689999-12182-3-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 02/10] hw: do not pass NULL to memory_region_init from instance_init List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, afaerber@suse.de, stefanha@redhat.com, ehabkost@redhat.com On 01/10/15 10:59, Markus Armbruster wrote: > From: Paolo Bonzini > > This causes the region to outlive the object, because it attaches the > region to /machine. This is not nice for the "realize" method, but > much worse for "instance_init" because it can cause dangling pointers > after a simple object_new/object_unref pair. > > Reported-by: Markus Armbruster > Signed-off-by: Paolo Bonzini > Message-Id: <1443530263-32340-3-git-send-email-pbonzini@redhat.com> > Reviewed-by: Peter Maydell > Tested-by: Markus Armbruster > Signed-off-by: Markus Armbruster > --- > hw/arm/pxa2xx.c | 2 +- > hw/display/cg3.c | 4 ++-- > hw/display/tcx.c | 2 +- > hw/misc/arm_integrator_debug.c | 2 +- > hw/misc/macio/cuda.c | 2 +- > hw/misc/macio/macio.c | 6 +++--- > hw/pcmcia/pxa2xx.c | 6 +++--- > 7 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c > index 164260a..79d22d9 100644 > --- a/hw/arm/pxa2xx.c > +++ b/hw/arm/pxa2xx.c > @@ -1958,7 +1958,7 @@ static void pxa2xx_fir_instance_init(Object *obj) > PXA2xxFIrState *s = PXA2XX_FIR(obj); > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > > - memory_region_init_io(&s->iomem, NULL, &pxa2xx_fir_ops, s, > + memory_region_init_io(&s->iomem, obj, &pxa2xx_fir_ops, s, > "pxa2xx-fir", 0x1000); > sysbus_init_mmio(sbd, &s->iomem); > sysbus_init_irq(sbd, &s->irq); > diff --git a/hw/display/cg3.c b/hw/display/cg3.c > index d2a0d97..e309fbe 100644 > --- a/hw/display/cg3.c > +++ b/hw/display/cg3.c > @@ -280,12 +280,12 @@ static void cg3_initfn(Object *obj) > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > CG3State *s = CG3(obj); > > - memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE, > + memory_region_init_ram(&s->rom, obj, "cg3.prom", FCODE_MAX_ROM_SIZE, > &error_fatal); > memory_region_set_readonly(&s->rom, true); > sysbus_init_mmio(sbd, &s->rom); > > - memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg", > + memory_region_init_io(&s->reg, obj, &cg3_reg_ops, s, "cg3.reg", > CG3_REG_SIZE); > sysbus_init_mmio(sbd, &s->reg); > } > diff --git a/hw/display/tcx.c b/hw/display/tcx.c > index 4635800..bf119bc 100644 > --- a/hw/display/tcx.c > +++ b/hw/display/tcx.c > @@ -944,7 +944,7 @@ static void tcx_initfn(Object *obj) > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > TCXState *s = TCX(obj); > > - memory_region_init_ram(&s->rom, NULL, "tcx.prom", FCODE_MAX_ROM_SIZE, > + memory_region_init_ram(&s->rom, OBJECT(s), "tcx.prom", FCODE_MAX_ROM_SIZE, > &error_fatal); > memory_region_set_readonly(&s->rom, true); > sysbus_init_mmio(sbd, &s->rom); I'd still use "obj" instead of "OBJECT(s)", even if the rest of the function is doing it differently ... but I guess we can also clean that up later ... So anyway, patch looks fine to me: Reviewed-by: Thomas Huth