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

[Wei Yang]

On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
>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.
>

In case there is no strong requirement to refactor the code. I would leave it
here.

>-- 
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me