diff mbox series

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

Message ID 20240102015808.132373-17-richard.henderson@linaro.org
State Superseded
Headers show
Series linux-user: Improve host and guest page size handling | expand

Commit Message

Richard Henderson Jan. 2, 2024, 1:57 a.m. UTC
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(-)

Comments

Pierrick Bouvier Jan. 8, 2024, 10:50 a.m. UTC | #1
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>
Ilya Leoshkevich Jan. 29, 2024, 2:45 p.m. UTC | #2
On Tue, Jan 02, 2024 at 12:57:51PM +1100, 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(-)

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
diff mbox series

Patch

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)