Re: [Qemu-devel] [PATCH v4 10/12] nvdimm: convert nvdimm_mr into a pointer

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 18 Jun 2018 16:25:34 +0200
David Hildenbrand <david@redhat.com> wrote:

> This way we can easily check if the region has already been inititalized
> without having to rely on the size of an uninitialized region being 0.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/nvdimm.c         | 9 +++++----
>  include/hw/mem/nvdimm.h | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7260c9c6b1..db7d8c3050 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -43,7 +43,7 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>      Error *local_err = NULL;
>      uint64_t value;
>  
> -    if (memory_region_size(&nvdimm->nvdimm_mr)) {
> +    if (nvdimm->nvdimm_mr) {
>          error_setg(&local_err, "cannot change property value");
>          goto out;
>      }
> @@ -75,7 +75,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>  
> -    return &nvdimm->nvdimm_mr;
> +    return nvdimm->nvdimm_mr;
>  }
>  
>  static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
> @@ -102,9 +102,10 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>          return;
>      }
>  
> -    memory_region_init_alias(&nvdimm->nvdimm_mr, OBJECT(dimm),
> +    nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
missed leak,
you need to free this manually in unrealize() in this patch but
considering that the next patches would move this allocation before
realize, it has to be move to finalize(). So make a note in commit
message explaining why finalize() is used prematurely.

probably no need to respin whole series, just send v5 of this patch
as reply here if change doesn't break series.

> +    memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
>                               "nvdimm-memory", mr, 0, pmem_size);
> -    nvdimm->nvdimm_mr.align = align;
> +    nvdimm->nvdimm_mr->align = align;
>  }
>  
>  /*
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 9340631cfc..c5c9b3c7f8 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -74,7 +74,7 @@ struct NVDIMMDevice {
>       * it's the PMEM region in NVDIMM device, which is presented to
>       * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
>       */
> -    MemoryRegion nvdimm_mr;
> +    MemoryRegion *nvdimm_mr;
>  
>      /*
>       * The 'on' value results in the unarmed flag set in ACPI NFIT,