Re: [Qemu-devel] [PATCH v5 02/10] hw: do not pass NULL to memory_region_init from instance_init

[Thomas Huth <thuth@redhat.com>]

On 01/10/15 10:59, Markus Armbruster wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> 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 <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <1443530263-32340-3-git-send-email-pbonzini@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Tested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  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 <thuth@redhat.com>