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

[Maxim Davydov <maxim.davydov@openvz.org>]

On 3/30/22 14:27, Vladimir Sementsov-Ogievskiy wrote:
> 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?
I tried to solve the problem with errors from request MemoryRegion (via 
*md_get_memory_region()) that was called immediately after 
object_new_with_class(). But it does seem to change the semantics. 
Perhaps better solution would be to ignore these errors or to add an 
exception to handle the object properties correctly.
>
>>   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,
Maxim Davydov