Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check

[David Hildenbrand]

On 08.08.19 04:38, Wei Yang wrote:
> On Thu, Aug 08, 2019 at 02:30:02AM +0000, Zeng, Star wrote:
>>> -----Original Message-----
>>> From: Wei Yang [mailto:address@hidden]
>>> Sent: Thursday, August 8, 2019 10:13 AM
>>> To: Zeng, Star <address@hidden>
>>> Cc: Wei Yang <address@hidden>; address@hidden;
>>> address@hidden; address@hidden; address@hidden
>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>> use goto for the last check
>>>
>>> On Thu, Aug 08, 2019 at 01:42:14AM +0000, Zeng, Star wrote:
>>>>> -----Original Message-----
>>>>> From: Qemu-devel [mailto:qemu-devel-
>>>>> bounces+star.zeng=address@hidden] On Behalf Of Wei Yang
>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>>>>> To: address@hidden
>>>>> Cc: address@hidden; address@hidden; Wei Yang
>>>>> <address@hidden>; address@hidden
>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
>>>>> use goto for the last check
>>>>>
>>>>> We are already at the last condition check.
>>>>>
>>>>> Signed-off-by: Wei Yang <address@hidden>
>>>>> Reviewed-by: Igor Mammedov <address@hidden>
>>>>> Reviewed-by: David Hildenbrand <address@hidden>
>>>>> ---
>>>>>  hw/mem/memory-device.c | 1 -
>>>>>  1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>>> index
>>>>> 5f2c408036..df3261b32a 100644
>>>>> --- a/hw/mem/memory-device.c
>>>>> +++ b/hw/mem/memory-device.c
>>>>> @@ -186,7 +186,6 @@ static uint64_t
>>>>> memory_device_get_free_addr(MachineState *ms,
>>>>>      if (!range_contains_range(&as, &new)) {
>>>>>          error_setg(errp, "could not find position in guest address space 
>>>>> for "
>>>>>                     "memory device - memory fragmented due to 
>>>>> alignments");
>>>>> -        goto out;
>>>>
>>>> Is it better to return 0 (or set new_addr to 0) for this error path and 
>>>> another
>>> remaining "goto out" path?
>>>>
>>>
>>> I may not get your point.
>>>
>>> We set errp which is handled in its caller. By doing so, the error is 
>>> propagated.
>>>
>>> Do I miss something?
>>
>> Yes, you are right. Currently, the caller is checking errp, but not the 
>> returned address, so there should be no issue.
>> But when you see other error paths, you will find they all return 0. To be 
>> aligned (return 0 when error), so just suggest also returning 0 for these 
>> two "goto out" error path. :)
>>
> 
> You may have some point.
> 
> Let's see whether others have the same taste, or we can refine it separately.
> 

I don't think that's necessary (callers really should check for errors
before using the return values), but I would also not object to that change.

-- 

Thanks,

David / dhildenb