VFIO: misc cleanups

[PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() return bool

Posted by Zhenzhong Duan 1 year, 9 months ago

This is to follow the coding standand to return bool if 'Error **'
is used to pass error.

The changed functions include:

iommufd_backend_connect
iommufd_backend_alloc_ioas

By this chance, simplify the functions a bit by avoiding duplicate
recordings, e.g., log through either error interface or trace, not
both.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/sysemu/iommufd.h |  6 +++---
 backends/iommufd.c       | 29 +++++++++++++----------------
 hw/vfio/iommufd.c        |  5 ++---
 backends/trace-events    |  4 ++--
 4 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 9af27ebd6c..293bfbe967 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -23,11 +23,11 @@ struct IOMMUFDBackend {
     /*< public >*/
 };
 
-int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
+bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
 void iommufd_backend_disconnect(IOMMUFDBackend *be);
 
-int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
-                               Error **errp);
+bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
+                                Error **errp);
 void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id);
 int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
                             ram_addr_t size, void *vaddr, bool readonly);
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 76a0204852..c506afbdac 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -72,24 +72,22 @@ static void iommufd_backend_class_init(ObjectClass *oc, void *data)
     object_class_property_add_str(oc, "fd", NULL, iommufd_backend_set_fd);
 }
 
-int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
+bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
 {
-    int fd, ret = 0;
+    int fd;
 
     if (be->owned && !be->users) {
         fd = qemu_open_old("/dev/iommu", O_RDWR);
         if (fd < 0) {
             error_setg_errno(errp, errno, "/dev/iommu opening failed");
-            ret = fd;
-            goto out;
+            return false;
         }
         be->fd = fd;
     }
     be->users++;
-out:
-    trace_iommufd_backend_connect(be->fd, be->owned,
-                                  be->users, ret);
-    return ret;
+
+    trace_iommufd_backend_connect(be->fd, be->owned, be->users);
+    return true;
 }
 
 void iommufd_backend_disconnect(IOMMUFDBackend *be)
@@ -106,25 +104,24 @@ out:
     trace_iommufd_backend_disconnect(be->fd, be->users);
 }
 
-int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
-                               Error **errp)
+bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
+                                Error **errp)
 {
-    int ret, fd = be->fd;
+    int fd = be->fd;
     struct iommu_ioas_alloc alloc_data  = {
         .size = sizeof(alloc_data),
         .flags = 0,
     };
 
-    ret = ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data);
-    if (ret) {
+    if (ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data)) {
         error_setg_errno(errp, errno, "Failed to allocate ioas");
-        return ret;
+        return false;
     }
 
     *ioas_id = alloc_data.out_ioas_id;
-    trace_iommufd_backend_alloc_ioas(fd, *ioas_id, ret);
+    trace_iommufd_backend_alloc_ioas(fd, *ioas_id);
 
-    return ret;
+    return true;
 }
 
 void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 6a446b16dc..554f9a6292 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -71,7 +71,7 @@ static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
         .flags = 0,
     };
 
-    if (iommufd_backend_connect(iommufd, errp)) {
+    if (!iommufd_backend_connect(iommufd, errp)) {
         return false;
     }
 
@@ -346,8 +346,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
     }
 
     /* Need to allocate a new dedicated container */
-    ret = iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp);
-    if (ret < 0) {
+    if (!iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp)) {
         goto err_alloc_ioas;
     }
 
diff --git a/backends/trace-events b/backends/trace-events
index d45c6e31a6..211e6f374a 100644
--- a/backends/trace-events
+++ b/backends/trace-events
@@ -7,11 +7,11 @@ dbus_vmstate_loading(const char *id) "id: %s"
 dbus_vmstate_saving(const char *id) "id: %s"
 
 # iommufd.c
-iommufd_backend_connect(int fd, bool owned, uint32_t users, int ret) "fd=%d owned=%d users=%d (%d)"
+iommufd_backend_connect(int fd, bool owned, uint32_t users) "fd=%d owned=%d users=%d"
 iommufd_backend_disconnect(int fd, uint32_t users) "fd=%d users=%d"
 iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d"
 iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, void *vaddr, bool readonly, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p readonly=%d (%d)"
 iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " Unmap nonexistent mapping: iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
 iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
-iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas, int ret) " iommufd=%d ioas=%d (%d)"
+iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
 iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
-- 
2.34.1

Re: [PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() return bool

Posted by Cédric Le Goater 1 year, 8 months ago

On 5/7/24 08:42, Zhenzhong Duan wrote:
> This is to follow the coding standand to return bool if 'Error **'
> is used to pass error.
> 
> The changed functions include:
> 
> iommufd_backend_connect
> iommufd_backend_alloc_ioas
> 
> By this chance, simplify the functions a bit by avoiding duplicate
> recordings, e.g., log through either error interface or trace, not
> both.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


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

Thanks,

C.


> ---
>   include/sysemu/iommufd.h |  6 +++---
>   backends/iommufd.c       | 29 +++++++++++++----------------
>   hw/vfio/iommufd.c        |  5 ++---
>   backends/trace-events    |  4 ++--
>   4 files changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index 9af27ebd6c..293bfbe967 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -23,11 +23,11 @@ struct IOMMUFDBackend {
>       /*< public >*/
>   };
>   
> -int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
> +bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
>   void iommufd_backend_disconnect(IOMMUFDBackend *be);
>   
> -int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> -                               Error **errp);
> +bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> +                                Error **errp);
>   void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id);
>   int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
>                               ram_addr_t size, void *vaddr, bool readonly);
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 76a0204852..c506afbdac 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -72,24 +72,22 @@ static void iommufd_backend_class_init(ObjectClass *oc, void *data)
>       object_class_property_add_str(oc, "fd", NULL, iommufd_backend_set_fd);
>   }
>   
> -int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
> +bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
>   {
> -    int fd, ret = 0;
> +    int fd;
>   
>       if (be->owned && !be->users) {
>           fd = qemu_open_old("/dev/iommu", O_RDWR);
>           if (fd < 0) {
>               error_setg_errno(errp, errno, "/dev/iommu opening failed");
> -            ret = fd;
> -            goto out;
> +            return false;
>           }
>           be->fd = fd;
>       }
>       be->users++;
> -out:
> -    trace_iommufd_backend_connect(be->fd, be->owned,
> -                                  be->users, ret);
> -    return ret;
> +
> +    trace_iommufd_backend_connect(be->fd, be->owned, be->users);
> +    return true;
>   }
>   
>   void iommufd_backend_disconnect(IOMMUFDBackend *be)
> @@ -106,25 +104,24 @@ out:
>       trace_iommufd_backend_disconnect(be->fd, be->users);
>   }
>   
> -int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> -                               Error **errp)
> +bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
> +                                Error **errp)
>   {
> -    int ret, fd = be->fd;
> +    int fd = be->fd;
>       struct iommu_ioas_alloc alloc_data  = {
>           .size = sizeof(alloc_data),
>           .flags = 0,
>       };
>   
> -    ret = ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data);
> -    if (ret) {
> +    if (ioctl(fd, IOMMU_IOAS_ALLOC, &alloc_data)) {
>           error_setg_errno(errp, errno, "Failed to allocate ioas");
> -        return ret;
> +        return false;
>       }
>   
>       *ioas_id = alloc_data.out_ioas_id;
> -    trace_iommufd_backend_alloc_ioas(fd, *ioas_id, ret);
> +    trace_iommufd_backend_alloc_ioas(fd, *ioas_id);
>   
> -    return ret;
> +    return true;
>   }
>   
>   void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 6a446b16dc..554f9a6292 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -71,7 +71,7 @@ static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
>           .flags = 0,
>       };
>   
> -    if (iommufd_backend_connect(iommufd, errp)) {
> +    if (!iommufd_backend_connect(iommufd, errp)) {
>           return false;
>       }
>   
> @@ -346,8 +346,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>       }
>   
>       /* Need to allocate a new dedicated container */
> -    ret = iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp);
> -    if (ret < 0) {
> +    if (!iommufd_backend_alloc_ioas(vbasedev->iommufd, &ioas_id, errp)) {
>           goto err_alloc_ioas;
>       }
>   
> diff --git a/backends/trace-events b/backends/trace-events
> index d45c6e31a6..211e6f374a 100644
> --- a/backends/trace-events
> +++ b/backends/trace-events
> @@ -7,11 +7,11 @@ dbus_vmstate_loading(const char *id) "id: %s"
>   dbus_vmstate_saving(const char *id) "id: %s"
>   
>   # iommufd.c
> -iommufd_backend_connect(int fd, bool owned, uint32_t users, int ret) "fd=%d owned=%d users=%d (%d)"
> +iommufd_backend_connect(int fd, bool owned, uint32_t users) "fd=%d owned=%d users=%d"
>   iommufd_backend_disconnect(int fd, uint32_t users) "fd=%d users=%d"
>   iommu_backend_set_fd(int fd) "pre-opened /dev/iommu fd=%d"
>   iommufd_backend_map_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, void *vaddr, bool readonly, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" addr=%p readonly=%d (%d)"
>   iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " Unmap nonexistent mapping: iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
>   iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" size=0x%"PRIx64" (%d)"
> -iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas, int ret) " iommufd=%d ioas=%d (%d)"
> +iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
>   iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"