diff mbox series

[v2,1/2] memory-device: not necessary to use goto for the last check

Message ID 20190730003740.20694-2-richardw.yang@linux.intel.com
State New
Headers show
Series refine memory_device_get_free_addr | expand

Commit Message

Wei Yang July 30, 2019, 12:37 a.m. UTC
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(-)

Comments

Zeng, Star Aug. 8, 2019, 1:42 a.m. UTC | #1
> -----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?


Thanks,
Star

>      }
>  out:
>      g_slist_free(list);
> --
> 2.17.1
>
Wei Yang Aug. 8, 2019, 2:13 a.m. UTC | #2
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?

>
>Thanks,
>Star
>
>>      }
>>  out:
>>      g_slist_free(list);
>> --
>> 2.17.1
>>
Zeng, Star Aug. 8, 2019, 2:30 a.m. UTC | #3
> -----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. :)

Thanks,
Star

> 
> >
> >Thanks,
> >Star
> >
> >>      }
> >>  out:
> >>      g_slist_free(list);
> >> --
> >> 2.17.1
> >>
> 
> --
> Wei Yang
> Help you, Help me
Wei Yang Aug. 8, 2019, 2:38 a.m. UTC | #4
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.

>Thanks,
>Star
>
>> 
>> >
>> >Thanks,
>> >Star
>> >
>> >>      }
>> >>  out:
>> >>      g_slist_free(list);
>> >> --
>> >> 2.17.1
>> >>
>> 
>> --
>> Wei Yang
>> Help you, Help me
David Hildenbrand Aug. 8, 2019, 7:06 a.m. UTC | #5
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.
Wei Yang Aug. 19, 2019, 2:38 a.m. UTC | #6
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: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.
>

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

>-- 
>
>Thanks,
>
>David / dhildenb
Zeng, Star Aug. 19, 2019, 5:32 a.m. UTC | #7
> -----Original Message-----
> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
> Sent: Monday, August 19, 2019 10:39 AM
> To: David Hildenbrand <david@redhat.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>; Zeng, Star
> <star.zeng@intel.com>; imammedo@redhat.com; qemu-devel@nongnu.org;
> 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 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: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.
> >
> 
> 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,
Star

> 
> >--
> >
> >Thanks,
> >
> >David / dhildenb
> 
> --
> Wei Yang
> Help you, Help me
Wei Yang Aug. 19, 2019, 6:36 a.m. UTC | #8
On Mon, Aug 19, 2019 at 05:32:14AM +0000, Zeng, Star wrote:
>
>
>> -----Original Message-----
>> From: Wei Yang [mailto:richardw.yang@linux.intel.com]
>> Sent: Monday, August 19, 2019 10:39 AM
>> To: David Hildenbrand <david@redhat.com>
>> Cc: Wei Yang <richardw.yang@linux.intel.com>; Zeng, Star
>> <star.zeng@intel.com>; imammedo@redhat.com; qemu-devel@nongnu.org;
>> 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 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: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.
>> >
>> 
>> 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
diff mbox series

Patch

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;
     }
 out:
     g_slist_free(list);