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

[Zeng, Star]

> -----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. :)

Thanks,
Star

> 
> >
> >Thanks,
> >Star
> >
> >>      }
> >>  out:
> >>      g_slist_free(list);
> >> --
> >> 2.17.1
> >>
> 
> --
> Wei Yang
> Help you, Help me