Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Laurent Vivier]

Le 13/03/2018 à 14:54, Peter Maydell a écrit :
> On 13 March 2018 at 13:30, Laurent Vivier  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

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Peter Maydell]

On 13 March 2018 at 13:30, Laurent Vivier  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

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Laurent Vivier]

Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>> From: Luke Shumaker 
>>
>> 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 
>> ---
>>  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

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Laurent Vivier]

Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
> From: Luke Shumaker 
> 
> 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 
> ---
>  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

Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Peter Maydell]

On 28 December 2017 at 18:08, Luke Shumaker  wrote:
> From: Luke Shumaker 
>
> 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 
> ---
>  linux-user/elfload.c | 34 +++---
>  1 file changed, 19 insertions(+), 15 deletions(-)
>

Reviewed-by: Peter Maydell 

thanks
-- PMM