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

[David Hildenbrand <david@redhat.com>]

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:richardw.yang@linux.intel.com]
>>> Sent: Thursday, August 8, 2019 10:13 AM
>>> To: Zeng, Star <star.zeng@intel.com>
>>> Cc: Wei Yang <richardw.yang@linux.intel.com>; qemu-devel@nongnu.org;
>>> imammedo@redhat.com; david@redhat.com; mst@redhat.com
>>> 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=intel.com@nongnu.org] On Behalf Of Wei Yang
>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
>>>>> To: qemu-devel@nongnu.org
>>>>> Cc: imammedo@redhat.com; david@redhat.com; Wei Yang
>>>>> <richardw.yang@linux.intel.com>; mst@redhat.com
>>>>> 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 <richardw.yang@linux.intel.com>
>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  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