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

[Wei Yang]

On Mon, Aug 19, 2019 at 05:32:14AM +0000, Zeng, Star wrote:
>
>
>> -----Original Message-----
>> From: Wei Yang [mailto:address@hidden]
>> Sent: Monday, August 19, 2019 10:39 AM
>> To: David Hildenbrand <address@hidden>
>> Cc: Wei Yang <address@hidden>; Zeng, Star
>> <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 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.
>
>It was just my suggestion. I am fine with any preference you and other experts 
>have.
>

Thanks

>Thanks,
>Star
>
>> 
>> >--
>> >
>> >Thanks,
>> >
>> >David / dhildenb
>> 
>> --
>> Wei Yang
>> Help you, Help me

-- 
Wei Yang
Help you, Help me