From: Luke Shumaker <lukeshu@parabola.nu>
Instead of doing
if (check1) {
if (check2) {
success;
}
}
retry;
Do a clearer
if (!check1) {
goto try_again;
}
if (!check2) {
goto try_again;
}
success;
try_again:
retry;
Besides being clearer, this makes it easier to insert more checks that
need to trigger a retry on check failure, or rearrange them, or anything
like that.
Because some indentation is changing, "ignore space change" may be useful
for viewing this patch.
Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
---
linux-user/elfload.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b560f5d6fe..5c0ad65611 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
}
/* Check to see if the address is valid. */
- if (!host_start || aligned_start == current_start) {
+ if (host_start && aligned_start != current_start) {
+ goto try_again;
+ }
+
#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
- /* On 32-bit ARM, we need to also be able to map the commpage. */
- int valid = init_guest_commpage(aligned_start - guest_start,
- aligned_size + guest_start);
- if (valid == 1) {
- break;
- } else if (valid == -1) {
- munmap((void *)real_start, real_size);
- return (unsigned long)-1;
- }
- /* valid == 0, so try again. */
-#else
- /* On other architectures, whatever we have here is fine. */
- break;
-#endif
+ /* On 32-bit ARM, we need to also be able to map the commpage. */
+ int valid = init_guest_commpage(aligned_start - guest_start,
+ aligned_size + guest_start);
+ if (valid == -1) {
+ munmap((void *)real_start, real_size);
+ return (unsigned long)-1;
+ } else if (valid == -1) {
+ goto try_again;
}
+#endif
+
+ /* If nothing has said `return -1` or `goto try_again` yet,
+ * then the address we have is good.
+ */
+ break;
+ try_again:
/* That address didn't work. Unmap and try a different one.
* The address the host picked because is typically right at
* the top of the host address space and leaves the guest with
--
2.15.1
On 28 December 2017 at 18:08, Luke Shumaker <lukeshu@lukeshu.com> wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Instead of doing
>
> if (check1) {
> if (check2) {
> success;
> }
> }
>
> retry;
>
> Do a clearer
>
> if (!check1) {
> goto try_again;
> }
>
> if (!check2) {
> goto try_again;
> }
>
> success;
>
> try_again:
> retry;
>
> Besides being clearer, this makes it easier to insert more checks that
> need to trigger a retry on check failure, or rearrange them, or anything
> like that.
>
> Because some indentation is changing, "ignore space change" may be useful
> for viewing this patch.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
> linux-user/elfload.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker <lukeshu@parabola.nu>
>
> Instead of doing
>
> if (check1) {
> if (check2) {
> success;
> }
> }
>
> retry;
>
> Do a clearer
>
> if (!check1) {
> goto try_again;
> }
>
> if (!check2) {
> goto try_again;
> }
>
> success;
>
> try_again:
> retry;
>
> Besides being clearer, this makes it easier to insert more checks that
> need to trigger a retry on check failure, or rearrange them, or anything
> like that.
>
> Because some indentation is changing, "ignore space change" may be useful
> for viewing this patch.
>
> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
> ---
> linux-user/elfload.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index b560f5d6fe..5c0ad65611 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
> }
>
> /* Check to see if the address is valid. */
> - if (!host_start || aligned_start == current_start) {
> + if (host_start && aligned_start != current_start) {
> + goto try_again;
> + }
> +
> #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
> - /* On 32-bit ARM, we need to also be able to map the commpage. */
> - int valid = init_guest_commpage(aligned_start - guest_start,
> - aligned_size + guest_start);
> - if (valid == 1) {
> - break;
> - } else if (valid == -1) {
> - munmap((void *)real_start, real_size);
> - return (unsigned long)-1;
> - }
> - /* valid == 0, so try again. */
> -#else
> - /* On other architectures, whatever we have here is fine. */
> - break;
> -#endif
> + /* On 32-bit ARM, we need to also be able to map the commpage. */
> + int valid = init_guest_commpage(aligned_start - guest_start,
> + aligned_size + guest_start);
> + if (valid == -1) {
> + munmap((void *)real_start, real_size);
> + return (unsigned long)-1;
> + } else if (valid == -1) {
I think it should be "if (valid == 0)" here.
> + goto try_again;
> }
> +#endif
> +
> + /* If nothing has said `return -1` or `goto try_again` yet,
> + * then the address we have is good.
> + */
> + break;
>
> + try_again:
> /* That address didn't work. Unmap and try a different one.
> * The address the host picked because is typically right at
> * the top of the host address space and leaves the guest with
>
Thanks,
Laurent
Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>> From: Luke Shumaker <lukeshu@parabola.nu>
>>
>> Instead of doing
>>
>> if (check1) {
>> if (check2) {
>> success;
>> }
>> }
>>
>> retry;
>>
>> Do a clearer
>>
>> if (!check1) {
>> goto try_again;
>> }
>>
>> if (!check2) {
>> goto try_again;
>> }
>>
>> success;
>>
>> try_again:
>> retry;
>>
>> Besides being clearer, this makes it easier to insert more checks that
>> need to trigger a retry on check failure, or rearrange them, or anything
>> like that.
>>
>> Because some indentation is changing, "ignore space change" may be useful
>> for viewing this patch.
>>
>> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
>> ---
>> linux-user/elfload.c | 34 +++++++++++++++++++---------------
>> 1 file changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index b560f5d6fe..5c0ad65611 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>> }
>>
>> /* Check to see if the address is valid. */
>> - if (!host_start || aligned_start == current_start) {
>> + if (host_start && aligned_start != current_start) {
>> + goto try_again;
>> + }
>> +
>> #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>> - /* On 32-bit ARM, we need to also be able to map the commpage. */
>> - int valid = init_guest_commpage(aligned_start - guest_start,
>> - aligned_size + guest_start);
>> - if (valid == 1) {
>> - break;
>> - } else if (valid == -1) {
>> - munmap((void *)real_start, real_size);
>> - return (unsigned long)-1;
>> - }
>> - /* valid == 0, so try again. */
>> -#else
>> - /* On other architectures, whatever we have here is fine. */
>> - break;
>> -#endif
>> + /* On 32-bit ARM, we need to also be able to map the commpage. */
>> + int valid = init_guest_commpage(aligned_start - guest_start,
>> + aligned_size + guest_start);
>> + if (valid == -1) {
>> + munmap((void *)real_start, real_size);
>> + return (unsigned long)-1;
>> + } else if (valid == -1) {
>
> I think it should be "if (valid == 0)" here.
Any comment?
Thanks,
Laurent
On 13 March 2018 at 13:30, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
>> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>>> --- a/linux-user/elfload.c
>>> +++ b/linux-user/elfload.c
>>> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>>> }
>>>
>>> /* Check to see if the address is valid. */
>>> - if (!host_start || aligned_start == current_start) {
>>> + if (host_start && aligned_start != current_start) {
>>> + goto try_again;
>>> + }
>>> +
>>> #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>>> - /* On 32-bit ARM, we need to also be able to map the commpage. */
>>> - int valid = init_guest_commpage(aligned_start - guest_start,
>>> - aligned_size + guest_start);
>>> - if (valid == 1) {
>>> - break;
>>> - } else if (valid == -1) {
>>> - munmap((void *)real_start, real_size);
>>> - return (unsigned long)-1;
>>> - }
>>> - /* valid == 0, so try again. */
>>> -#else
>>> - /* On other architectures, whatever we have here is fine. */
>>> - break;
>>> -#endif
>>> + /* On 32-bit ARM, we need to also be able to map the commpage. */
>>> + int valid = init_guest_commpage(aligned_start - guest_start,
>>> + aligned_size + guest_start);
>>> + if (valid == -1) {
>>> + munmap((void *)real_start, real_size);
>>> + return (unsigned long)-1;
>>> + } else if (valid == -1) {
>>
>> I think it should be "if (valid == 0)" here.
>
> Any comment?
Yes, I think I agree.
thanks
-- PMM
Le 13/03/2018 à 14:54, Peter Maydell a écrit :
> On 13 March 2018 at 13:30, Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
>>> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>>>> --- a/linux-user/elfload.c
>>>> +++ b/linux-user/elfload.c
>>>> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>>>> }
>>>>
>>>> /* Check to see if the address is valid. */
>>>> - if (!host_start || aligned_start == current_start) {
>>>> + if (host_start && aligned_start != current_start) {
>>>> + goto try_again;
>>>> + }
>>>> +
>>>> #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>>>> - /* On 32-bit ARM, we need to also be able to map the commpage. */
>>>> - int valid = init_guest_commpage(aligned_start - guest_start,
>>>> - aligned_size + guest_start);
>>>> - if (valid == 1) {
>>>> - break;
>>>> - } else if (valid == -1) {
>>>> - munmap((void *)real_start, real_size);
>>>> - return (unsigned long)-1;
>>>> - }
>>>> - /* valid == 0, so try again. */
>>>> -#else
>>>> - /* On other architectures, whatever we have here is fine. */
>>>> - break;
>>>> -#endif
>>>> + /* On 32-bit ARM, we need to also be able to map the commpage. */
>>>> + int valid = init_guest_commpage(aligned_start - guest_start,
>>>> + aligned_size + guest_start);
>>>> + if (valid == -1) {
>>>> + munmap((void *)real_start, real_size);
>>>> + return (unsigned long)-1;
>>>> + } else if (valid == -1) {
>>>
>>> I think it should be "if (valid == 0)" here.
>>
>> Any comment?
>
> Yes, I think I agree.
>
Applied to my 'linux-user-for-2.12' branch with this change.
Thanks,
Laurent
© 2016 - 2025 Red Hat, Inc.