Re: [PATCH v1 3/9] mem: appropriate handling getting mem region

[Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>]

29.03.2022 00:15, Maxim Davydov wrote:
> Attempt to get memory region if the device doesn't have hostmem may not be
> an error. This can be happen immediately after initialization (getting
> value without default one).
> 
> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
> ---
>   hw/i386/sgx-epc.c | 5 ++++-
>   hw/mem/nvdimm.c   | 6 ++++++
>   hw/mem/pc-dimm.c  | 5 +++++
>   3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
> index d664829d35..1a4c8acdcc 100644
> --- a/hw/i386/sgx-epc.c
> +++ b/hw/i386/sgx-epc.c
> @@ -121,9 +121,12 @@ static MemoryRegion *sgx_epc_md_get_memory_region(MemoryDeviceState *md,
>   {
>       SGXEPCDevice *epc = SGX_EPC(md);
>       HostMemoryBackend *hostmem;
> +    DeviceState *dev = DEVICE(epc);
>   
>       if (!epc->hostmem) {
> -        error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> +        if (dev->realized) {
> +            error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> +        }
>           return NULL;
>       }

I can't judge, is it really and error or not.

But the way you change the logic is not correct, as you change the semantics:

Old semantics: on error return NULL and set errp, on success return non-NULL and not set errp

New semantics: on error return NULL and set errp, on success return anything (may be NULL) and not set errp.

Callers are not prepared to this. For example, look at memory_device_unplug:
it does

   mr = mdc->get_memory_region(md, &error_abort);

assume it returns NULL, which is not an error (so we don't crash on error_abort)

and then pass mr  to memory_region_del_subregion(), which in turn access mr->container, which will crash if mr is NULL.

Most probably the situation I describe is not possible, but I just want to illustrate the idea.

Moreover, in QEMU functions which has "Error **errp" argument and return pointer are recommended to return NULL on failure and nonNULL on success. In other words, return value of function with "Error **errp" argument should report success/failure information. And having NULL as possible success return value is not recommended, as it's ambiguous and leads to bugs (see big comment at start of include/qapi/error.h).

So, if it's really needed to change the semantics in such not-recommended way, you should check that all callers are OK with it and also describe new semantics in a comment near get_memory_region declaration. But better is deal with returned error as it is.. What is an exact problem you trying to solve with this commit?

>   
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7c7d777781..61e77e5476 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -166,9 +166,15 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
>                                                    Error **errp)
>   {
>       NVDIMMDevice *nvdimm = NVDIMM(md);
> +    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
>       Error *local_err = NULL;
>   
>       if (!nvdimm->nvdimm_mr) {
> +        /* Not error if we try get memory region after init */
> +        if (!dimm->hostmem) {
> +            return NULL;
> +        }
> +
>           nvdimm_prepare_memory_region(nvdimm, &local_err);
>           if (local_err) {
>               error_propagate(errp, local_err);
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index f27e1a11ba..6fd74de97f 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -240,6 +240,11 @@ static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
>   static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
>                                                     Error **errp)
>   {
> +    PCDIMMDevice *dimm = PC_DIMM(md);
> +    /* Not error if we try get memory region after init */
> +    if (!dimm->hostmem) {
> +        return NULL;
> +    }
>       return pc_dimm_get_memory_region(PC_DIMM(md), errp);
>   }
>   


-- 
Best regards,
Vladimir