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 <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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