Re: [PATCH v3 16/33] linux-user: Split out target_mmap__locked

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 1/2/24 05:57, Richard Henderson wrote:
> All "goto fail" may be transformed to "return -1".
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/mmap.c | 62 ++++++++++++++++++++++++++---------------------
>   1 file changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index d11f758d07..b4c3cc65aa 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -490,9 +490,9 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong align)
>       }
>   }
>   
> -/* NOTE: all the constants are the HOST ones */
> -abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
> -                     int flags, int fd, off_t offset)
> +static abi_long target_mmap__locked(abi_ulong start, abi_ulong len,
> +                                    int target_prot, int flags,
> +                                    int fd, off_t offset)
>   {
>       int host_page_size = qemu_real_host_page_size();
>       abi_ulong ret, last, real_start, real_last, retaddr, host_len;
> @@ -500,30 +500,27 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>       int page_flags;
>       off_t host_offset;
>   
> -    mmap_lock();
> -    trace_target_mmap(start, len, target_prot, flags, fd, offset);
> -
>       if (!len) {
>           errno = EINVAL;
> -        goto fail;
> +        return -1;
>       }
>   
>       page_flags = validate_prot_to_pageflags(target_prot);
>       if (!page_flags) {
>           errno = EINVAL;
> -        goto fail;
> +        return -1;
>       }
>   
>       /* Also check for overflows... */
>       len = TARGET_PAGE_ALIGN(len);
>       if (!len) {
>           errno = ENOMEM;
> -        goto fail;
> +        return -1;
>       }
>   
>       if (offset & ~TARGET_PAGE_MASK) {
>           errno = EINVAL;
> -        goto fail;
> +        return -1;
>       }
>   
>       /*
> @@ -553,7 +550,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>           start = mmap_find_vma(real_start, host_len, TARGET_PAGE_SIZE);
>           if (start == (abi_ulong)-1) {
>               errno = ENOMEM;
> -            goto fail;
> +            return -1;
>           }
>       }
>   
> @@ -574,7 +571,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>           struct stat sb;
>   
>           if (fstat(fd, &sb) == -1) {
> -            goto fail;
> +            return -1;
>           }
>   
>           /* Are we trying to create a map beyond EOF?.  */
> @@ -601,7 +598,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>           p = mmap(g2h_untagged(start), host_len, host_prot,
>                    flags | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
>           if (p == MAP_FAILED) {
> -            goto fail;
> +            return -1;
>           }
>           /* update start so that it points to the file position at 'offset' */
>           host_start = (uintptr_t)p;
> @@ -610,7 +607,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>                        flags | MAP_FIXED, fd, host_offset);
>               if (p == MAP_FAILED) {
>                   munmap(g2h_untagged(start), host_len);
> -                goto fail;
> +                return -1;
>               }
>               host_start += offset - host_offset;
>           }
> @@ -621,7 +618,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>       } else {
>           if (start & ~TARGET_PAGE_MASK) {
>               errno = EINVAL;
> -            goto fail;
> +            return -1;
>           }
>           last = start + len - 1;
>           real_last = ROUND_UP(last, host_page_size) - 1;
> @@ -633,14 +630,14 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>            */
>           if (last < start || !guest_range_valid_untagged(start, len)) {
>               errno = ENOMEM;
> -            goto fail;
> +            return -1;
>           }
>   
>           if (flags & MAP_FIXED_NOREPLACE) {
>               /* Validate that the chosen range is empty. */
>               if (!page_check_range_empty(start, last)) {
>                   errno = EEXIST;
> -                goto fail;
> +                return -1;
>               }
>   
>               /*
> @@ -671,17 +668,17 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>               if ((flags & MAP_TYPE) == MAP_SHARED
>                   && (target_prot & PROT_WRITE)) {
>                   errno = EINVAL;
> -                goto fail;
> +                return -1;
>               }
>               retaddr = target_mmap(start, len, target_prot | PROT_WRITE,
>                                     (flags & (MAP_FIXED | MAP_FIXED_NOREPLACE))
>                                     | MAP_PRIVATE | MAP_ANONYMOUS,
>                                     -1, 0);
>               if (retaddr == -1) {
> -                goto fail;
> +                return -1;
>               }
>               if (pread(fd, g2h_untagged(start), len, offset) == -1) {
> -                goto fail;
> +                return -1;
>               }
>               if (!(target_prot & PROT_WRITE)) {
>                   ret = target_mprotect(start, len, target_prot);
> @@ -696,14 +693,14 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>                   /* one single host page */
>                   if (!mmap_frag(real_start, start, last,
>                                  target_prot, flags, fd, offset)) {
> -                    goto fail;
> +                    return -1;
>                   }
>                   goto the_end1;
>               }
>               if (!mmap_frag(real_start, start,
>                              real_start + host_page_size - 1,
>                              target_prot, flags, fd, offset)) {
> -                goto fail;
> +                return -1;
>               }
>               real_start += host_page_size;
>           }
> @@ -713,7 +710,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>               if (!mmap_frag(real_page, real_page, last,
>                              target_prot, flags, fd,
>                              offset + real_page - start)) {
> -                goto fail;
> +                return -1;
>               }
>               real_last -= host_page_size;
>           }
> @@ -739,7 +736,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>                       munmap(p, len1);
>                       errno = EEXIST;
>                   }
> -                goto fail;
> +                return -1;
>               }
>               passthrough_start = real_start;
>               passthrough_last = real_last;
> @@ -773,11 +770,22 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
>               qemu_log_unlock(f);
>           }
>       }
> -    mmap_unlock();
>       return start;
> -fail:
> +}
> +
> +/* NOTE: all the constants are the HOST ones */
> +abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot,
> +                     int flags, int fd, off_t offset)
> +{
> +    abi_long ret;
> +
> +    trace_target_mmap(start, len, target_prot, flags, fd, offset);
> +    mmap_lock();
> +
> +    ret = target_mmap__locked(start, len, target_prot, flags, fd, offset);
> +
>       mmap_unlock();
> -    return -1;
> +    return ret;
>   }
>   
>   static int mmap_reserve_or_unmap(abi_ulong start, abi_ulong len)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>