Move error handling code to the end of the function so that it can more
easily be shared by new mmap failure conditions.  No functional change
intended.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/helpers.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index ea15c79db0a3..b9e606e364a2 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -395,7 +395,7 @@ static void vfio_subregion_unmap(VFIORegion *region, int index)
 
 int vfio_region_mmap(VFIORegion *region)
 {
-    int i, prot = 0;
+    int i, ret, prot = 0;
     char *name;
 
     if (!region->mem) {
@@ -411,22 +411,8 @@ int vfio_region_mmap(VFIORegion *region)
                                      region->fd_offset +
                                      region->mmaps[i].offset);
         if (region->mmaps[i].mmap == MAP_FAILED) {
-            int ret = -errno;
-
-            trace_vfio_region_mmap_fault(memory_region_name(region->mem), i,
-                                         region->fd_offset +
-                                         region->mmaps[i].offset,
-                                         region->fd_offset +
-                                         region->mmaps[i].offset +
-                                         region->mmaps[i].size - 1, ret);
-
-            region->mmaps[i].mmap = NULL;
-
-            for (i--; i >= 0; i--) {
-                vfio_subregion_unmap(region, i);
-            }
-
-            return ret;
+            ret = -errno;
+            goto no_mmap;
         }
 
         name = g_strdup_printf("%s mmaps[%d]",
@@ -446,6 +432,20 @@ int vfio_region_mmap(VFIORegion *region)
     }
 
     return 0;
+
+no_mmap:
+    trace_vfio_region_mmap_fault(memory_region_name(region->mem), i,
+                                 region->fd_offset + region->mmaps[i].offset,
+                                 region->fd_offset + region->mmaps[i].offset +
+                                 region->mmaps[i].size - 1, ret);
+
+    region->mmaps[i].mmap = NULL;
+
+    for (i--; i >= 0; i--) {
+        vfio_subregion_unmap(region, i);
+    }
+
+    return ret;
 }
 
 void vfio_region_unmap(VFIORegion *region)
-- 
2.46.2

On 10/22/24 22:08, Alex Williamson wrote:
> Move error handling code to the end of the function so that it can more
> easily be shared by new mmap failure conditions.  No functional change
> intended.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/helpers.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index ea15c79db0a3..b9e606e364a2 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -395,7 +395,7 @@ static void vfio_subregion_unmap(VFIORegion *region, int index)
>   
>   int vfio_region_mmap(VFIORegion *region)
>   {
> -    int i, prot = 0;
> +    int i, ret, prot = 0;
>       char *name;
>   
>       if (!region->mem) {
> @@ -411,22 +411,8 @@ int vfio_region_mmap(VFIORegion *region)
>                                        region->fd_offset +
>                                        region->mmaps[i].offset);
>           if (region->mmaps[i].mmap == MAP_FAILED) {
> -            int ret = -errno;
> -
> -            trace_vfio_region_mmap_fault(memory_region_name(region->mem), i,
> -                                         region->fd_offset +
> -                                         region->mmaps[i].offset,
> -                                         region->fd_offset +
> -                                         region->mmaps[i].offset +
> -                                         region->mmaps[i].size - 1, ret);
> -
> -            region->mmaps[i].mmap = NULL;
> -
> -            for (i--; i >= 0; i--) {
> -                vfio_subregion_unmap(region, i);
> -            }
> -
> -            return ret;
> +            ret = -errno;
> +            goto no_mmap;
>           }
>   
>           name = g_strdup_printf("%s mmaps[%d]",
> @@ -446,6 +432,20 @@ int vfio_region_mmap(VFIORegion *region)
>       }
>   
>       return 0;
> +
> +no_mmap:
> +    trace_vfio_region_mmap_fault(memory_region_name(region->mem), i,
> +                                 region->fd_offset + region->mmaps[i].offset,
> +                                 region->fd_offset + region->mmaps[i].offset +
> +                                 region->mmaps[i].size - 1, ret);
> +
> +    region->mmaps[i].mmap = NULL;
> +
> +    for (i--; i >= 0; i--) {
> +        vfio_subregion_unmap(region, i);
> +    }
> +
> +    return ret;
>   }
>   
>   void vfio_region_unmap(VFIORegion *region)

On Tue, Oct 22, 2024 at 02:08:28PM -0600, Alex Williamson wrote:
> Move error handling code to the end of the function so that it can more
> easily be shared by new mmap failure conditions.  No functional change
> intended.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  hw/vfio/helpers.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index ea15c79db0a3..b9e606e364a2 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -395,7 +395,7 @@ static void vfio_subregion_unmap(VFIORegion *region, int index)
>  
>  int vfio_region_mmap(VFIORegion *region)
>  {
> -    int i, prot = 0;
> +    int i, ret, prot = 0;
>      char *name;
>  
>      if (!region->mem) {
> @@ -411,22 +411,8 @@ int vfio_region_mmap(VFIORegion *region)
>                                       region->fd_offset +
>                                       region->mmaps[i].offset);
>          if (region->mmaps[i].mmap == MAP_FAILED) {
> -            int ret = -errno;
> -
> -            trace_vfio_region_mmap_fault(memory_region_name(region->mem), i,
> -                                         region->fd_offset +
> -                                         region->mmaps[i].offset,
> -                                         region->fd_offset +
> -                                         region->mmaps[i].offset +
> -                                         region->mmaps[i].size - 1, ret);
> -
> -            region->mmaps[i].mmap = NULL;
> -
> -            for (i--; i >= 0; i--) {
> -                vfio_subregion_unmap(region, i);
> -            }
> -
> -            return ret;
> +            ret = -errno;
> +            goto no_mmap;
>          }
>  
>          name = g_strdup_printf("%s mmaps[%d]",
> @@ -446,6 +432,20 @@ int vfio_region_mmap(VFIORegion *region)
>      }
>  
>      return 0;
> +
> +no_mmap:
> +    trace_vfio_region_mmap_fault(memory_region_name(region->mem), i,
> +                                 region->fd_offset + region->mmaps[i].offset,
> +                                 region->fd_offset + region->mmaps[i].offset +
> +                                 region->mmaps[i].size - 1, ret);

While at it, maybe when moving we can rename s/fault/fail/ or some other
words to avoid "mmap" + "fault" being together. :) Totally not a huge deal.

Reviewed-by: Peter Xu <peterx@redhat.com>

> +
> +    region->mmaps[i].mmap = NULL;
> +
> +    for (i--; i >= 0; i--) {
> +        vfio_subregion_unmap(region, i);
> +    }
> +
> +    return ret;
>  }
>  
>  void vfio_region_unmap(VFIORegion *region)
> -- 
> 2.46.2
> 

-- 
Peter Xu