dma: use ERROR_INT()

The first in a series excising the use of the "return -errno" idiom. This is a
non-standard usage, and in userspace, we have "errno" for delivering side-band
error values. As there have been multiple bugs from not using standard error
return methods like -1+errno or NULL+errno, let's do that.

Signed-off-by: John Levon <john.levon@nutanix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@nutanix.com>

7 files changed, 64 insertions, 58 deletions

diff --git a/lib/dma.c b/lib/dma.c
index 82bb8c2..2aad1a8 100644
--- a/lib/dma.c
+++ b/lib/dma.c
@@ -156,11 +156,11 @@ MOCK_DEFINE(dma_controller_remove_region)(dma_controller_t *dma,
 
         err = dma_unregister(data, &region->info);
         if (err != 0) {
+            err = errno;
             vfu_log(dma->vfu_ctx, LOG_ERR,
-                   "failed to dma_unregister() DMA region [%p, %p): %s",
-                   region->info.iova.iov_base, iov_end(&region->info.iova),
-                   strerror(err));
-            return -err;
+                   "failed to dma_unregister() DMA region [%p, %p): %m",
+                   region->info.iova.iov_base, iov_end(&region->info.iova));
+            return ERROR_INT(err);
         }
 
         assert(region->refcnt == 0);
@@ -174,7 +174,7 @@ MOCK_DEFINE(dma_controller_remove_region)(dma_controller_t *dma,
         array_remove(&dma->regions, sizeof (*region), idx, &dma->nregions);
         return 0;
     }
-    return -ENOENT;
+    return ERROR_INT(ENOENT);
 }
 
 void
@@ -229,7 +229,7 @@ dma_map_region(dma_controller_t *dma, dma_memory_region_t *region)
                      region->fd, offset);
 
     if (mmap_base == MAP_FAILED) {
-        return -errno;
+        return -1;
     }
 
     // Do not dump.
@@ -258,7 +258,6 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
     int page_size = 0;
     char rstr[1024];
     int idx;
-    int ret;
 
     assert(dma != NULL);
 
@@ -274,8 +273,7 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
             if (offset != region->offset) {
                 vfu_log(dma->vfu_ctx, LOG_ERR, "bad offset for new DMA region "
                         "%s; existing=%#lx", rstr, region->offset);
-                ret = -EINVAL;
-                goto out;
+                return ERROR_INT(EINVAL);
             }
             if (!fds_are_same_file(region->fd, fd)) {
                 /*
@@ -286,14 +284,12 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
                  */
                 vfu_log(dma->vfu_ctx, LOG_ERR, "bad fd for new DMA region %s; "
                         "existing=%d", rstr, region->fd);
-                ret = -EINVAL;
-                goto out;
+                return ERROR_INT(EINVAL);
             }
             if (region->info.prot != prot) {
                 vfu_log(dma->vfu_ctx, LOG_ERR, "bad prot for new DMA region "
                         "%s; existing=%#x", rstr, region->info.prot);
-                ret = -EINVAL;
-                goto out;
+                return ERROR_INT(EINVAL);
             }
             return idx;
         }
@@ -306,15 +302,13 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
             vfu_log(dma->vfu_ctx, LOG_INFO, "new DMA region %s overlaps with "
                     "DMA region [%p, %p)", rstr, region->info.iova.iov_base,
                     iov_end(&region->info.iova));
-            ret = -EINVAL;
-            goto out;
+            return ERROR_INT(EINVAL);
         }
     }
 
     if (dma->nregions == dma->max_regions) {
         vfu_log(dma->vfu_ctx, LOG_ERR, "hit max regions %d", dma->max_regions);
-        ret = -EINVAL;
-        goto out;
+        return ERROR_INT(EINVAL);
     }
 
     idx = dma->nregions;
@@ -324,8 +318,7 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
         page_size = fd_get_blocksize(fd);
         if (page_size < 0) {
             vfu_log(dma->vfu_ctx, LOG_ERR, "bad page size %d", page_size);
-            ret = -EINVAL;
-            goto out;
+            return ERROR_INT(EINVAL);
         }
     }
     page_size = MAX(page_size, getpagesize());
@@ -340,26 +333,24 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
     region->fd = fd;
 
     if (fd != -1) {
-        ret = dma_map_region(dma, region);
+        int ret = dma_map_region(dma, region);
 
         if (ret != 0) {
+            ret = errno;
             vfu_log(dma->vfu_ctx, LOG_ERR,
-                   "failed to memory map DMA region %s: %s", rstr,
-                   strerror(-ret));
+                   "failed to memory map DMA region %s: %m", rstr);
 
             if (close(region->fd) == -1) {
                 vfu_log(dma->vfu_ctx, LOG_WARNING,
                         "failed to close fd %d: %m", region->fd);
             }
-            goto out;
+
+            return ERROR_INT(ret);
         }
     }
 
-    ret = idx;
     dma->nregions++;
-
-out:
-    return ret;
+    return idx;
 }
 
 int
@@ -407,8 +398,7 @@ out:
     if (!found) {
         // There is still a region which was not found.
         assert(len > 0);
-        errno = ENOENT;
-        return -1;
+        return ERROR_INT(ENOENT);
     } else if (cnt > max_sg) {
         cnt = -cnt - 1;
     }
@@ -420,10 +410,10 @@ static ssize_t
 get_bitmap_size(size_t region_size, size_t pgsize)
 {
     if (pgsize == 0) {
-        return -EINVAL;
+        return ERROR_INT(EINVAL);
     }
     if (region_size < pgsize) {
-        return -EINVAL;
+        return ERROR_INT(EINVAL);
     }
     size_t nr_pages = (region_size / pgsize) + (region_size % pgsize != 0);
     return (nr_pages / CHAR_BIT) + (nr_pages % CHAR_BIT != 0);
@@ -431,22 +421,22 @@ get_bitmap_size(size_t region_size, size_t pgsize)
 
 int dma_controller_dirty_page_logging_start(dma_controller_t *dma, size_t pgsize)
 {
-    int i;
+    size_t i;
 
     assert(dma != NULL);
 
     if (pgsize == 0) {
-        return -EINVAL;
+        return ERROR_INT(EINVAL);
     }
 
     if (dma->dirty_pgsize > 0) {
         if (dma->dirty_pgsize != pgsize) {
-            return -EINVAL;
+            return ERROR_INT(EINVAL);
         }
         return 0;
     }
 
-    for (i = 0; i < dma->nregions; i++) {
+    for (i = 0; i < (size_t)dma->nregions; i++) {
         dma_memory_region_t *region = &dma->regions[i];
         ssize_t bitmap_size;
 
@@ -457,27 +447,30 @@ int dma_controller_dirty_page_logging_start(dma_controller_t *dma, size_t pgsize
         }
         region->dirty_bitmap = calloc(bitmap_size, sizeof(char));
         if (region->dirty_bitmap == NULL) {
-            int j, ret = -errno;
+            int ret = errno;
+            size_t j;
+
             for (j = 0; j < i; j++) {
                 region = &dma->regions[j];
                 free(region->dirty_bitmap);
                 region->dirty_bitmap = NULL;
             }
-            return ret;
+            return ERROR_INT(ret);
         }
     }
     dma->dirty_pgsize = pgsize;
     return 0;
 }
 
-int dma_controller_dirty_page_logging_stop(dma_controller_t *dma)
+void
+dma_controller_dirty_page_logging_stop(dma_controller_t *dma)
 {
     int i;
 
     assert(dma != NULL);
 
     if (dma->dirty_pgsize == 0) {
-        return 0;
+        return;
     }
 
     for (i = 0; i < dma->nregions; i++) {
@@ -485,7 +478,6 @@ int dma_controller_dirty_page_logging_stop(dma_controller_t *dma)
         dma->regions[i].dirty_bitmap = NULL;
     }
     dma->dirty_pgsize = 0;
-    return 0;
 }
 
 int
@@ -507,11 +499,11 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
      */
     ret = dma_addr_to_sg(dma, addr, len, &sg, 1, PROT_NONE);
     if (ret != 1 || sg.dma_addr != addr || sg.length != len) {
-        return -ENOTSUP;
+        return ERROR_INT(ENOTSUP);
     }
 
     if (pgsize != dma->dirty_pgsize) {
-        return -EINVAL;
+        return ERROR_INT(EINVAL);
     }
 
     bitmap_size = get_bitmap_size(len, pgsize);
@@ -524,7 +516,7 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
      * expects to receive.
      */
     if (size != (size_t)bitmap_size) {
-        return -EINVAL;
+        return ERROR_INT(EINVAL);
     }
 
     region = &dma->regions[sg.region];
diff --git a/lib/dma.h b/lib/dma.h
index 5e086a0..861b8d3 100644
--- a/lib/dma.h
+++ b/lib/dma.h
@@ -76,6 +76,7 @@
 
 #include "libvfio-user.h"
 #include "common.h"
+#include "private.h"
 
 #define iov_end(iov) ((iov)->iov_base + (iov)->iov_len)
 
@@ -89,7 +90,7 @@ typedef struct {
     char *dirty_bitmap;         // Dirty page bitmap
 } dma_memory_region_t;
 
-typedef struct {
+typedef struct dma_controller {
     int max_regions;
     int nregions;
     struct vfu_ctx *vfu_ctx;
@@ -187,8 +188,7 @@ dma_init_sg(const dma_controller_t *dma, dma_sg_t *sg, vfu_dma_addr_t dma_addr,
     const dma_memory_region_t *const region = &dma->regions[region_index];
 
     if ((prot & PROT_WRITE) && !(region->info.prot & PROT_WRITE)) {
-        errno = EACCES;
-        return -1;
+        return ERROR_INT(EACCES);
     }
 
     sg->dma_addr = region->info.iova.iov_base;
@@ -260,12 +260,12 @@ dma_map_sg(dma_controller_t *dma, const dma_sg_t *sg, struct iovec *iov,
 
     for (i = 0; i < cnt; i++) {
         if (sg[i].region >= dma->nregions) {
-            return -EINVAL;
+            return ERROR_INT(EINVAL);
         }
         region = &dma->regions[sg[i].region];
 
         if (region->info.vaddr == NULL) {
-            return -EFAULT;
+            return ERROR_INT(EFAULT);
         }
 
         vfu_log(dma->vfu_ctx, LOG_DEBUG, "map %p-%p",
@@ -304,13 +304,12 @@ dma_unmap_sg(dma_controller_t *dma, const dma_sg_t *sg,
                 sg[i].dma_addr + sg[i].offset + sg[i].length);
         r->refcnt--;
     }
-    return;
 }
 
 int
 dma_controller_dirty_page_logging_start(dma_controller_t *dma, size_t pgsize);
 
-int
+void
 dma_controller_dirty_page_logging_stop(dma_controller_t *dma);
 
 int
diff --git a/lib/irq.c b/lib/irq.c
index bc48d4f..1dec459 100644
--- a/lib/irq.c
+++ b/lib/irq.c
@@ -30,8 +30,10 @@
  *
  */
 
+#include <assert.h>
 #include <errno.h>
 #include <limits.h>
+#include <stdlib.h>
 #include <sys/eventfd.h>
 
 #include "irq.h"
diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index 0440351..f4aa804 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -532,11 +532,12 @@ handle_dma_map_or_unmap(vfu_ctx_t *vfu_ctx, uint32_t size, bool map,
                                             region->size, fd, region->offset,
                                             region->prot);
             if (ret < 0) {
+                ret = -errno;
+                vfu_log(vfu_ctx, LOG_ERR, "failed to add DMA region %s: %m",
+                        rstr);
                 if (fd != -1) {
                     close(fd);
                 }
-                vfu_log(vfu_ctx, LOG_ERR, "failed to add DMA region %s: %s",
-                        rstr, strerror(-ret));
                 break;
             }
 
@@ -553,8 +554,9 @@ handle_dma_map_or_unmap(vfu_ctx_t *vfu_ctx, uint32_t size, bool map,
                                                vfu_ctx->dma_unregister,
                                                vfu_ctx);
             if (ret < 0) {
-                vfu_log(vfu_ctx, LOG_ERR, "failed to remove DMA region %s: %s",
-                        rstr, strerror(-ret));
+                ret = -errno;
+                vfu_log(vfu_ctx, LOG_ERR, "failed to remove DMA region %s: %m",
+                        rstr);
                 break;
             }
         }
@@ -603,6 +605,7 @@ handle_dirty_pages_get(vfu_ctx_t *vfu_ctx,
                                             r->bitmap.size,
                                             (char**)&((*iovecs)[i].iov_base));
         if (ret != 0) {
+            ret = -errno;
             goto out;
         }
         (*iovecs)[i].iov_len = r->bitmap.size;
@@ -637,8 +640,12 @@ MOCK_DEFINE(handle_dirty_pages)(vfu_ctx_t *vfu_ctx, uint32_t size,
     if (dirty_bitmap->flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) {
         ret = dma_controller_dirty_page_logging_start(vfu_ctx->dma,
                                                       migration_get_pgsize(vfu_ctx->migration));
+        if (ret == -1) {
+            ret = -errno;
+        }
     } else if (dirty_bitmap->flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) {
-        ret = dma_controller_dirty_page_logging_stop(vfu_ctx->dma);
+        dma_controller_dirty_page_logging_stop(vfu_ctx->dma);
+        ret = 0;
     } else if (dirty_bitmap->flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) {
         ret = handle_dirty_pages_get(vfu_ctx, iovecs, nr_iovecs,
                                      (struct vfio_iommu_type1_dirty_bitmap_get*)(dirty_bitmap + 1),
@@ -1520,7 +1527,7 @@ vfu_map_sg(vfu_ctx_t *vfu_ctx, const dma_sg_t *sg,
 
     ret = dma_map_sg(vfu_ctx->dma, sg, iov, cnt);
     if (ret < 0) {
-        return ERROR_INT(-ret);
+        return -1;
     }
 
     return 0;
diff --git a/lib/pci.c b/lib/pci.c
index 39470d9..54e90f9 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -33,6 +33,7 @@
 #include <assert.h>
 #include <errno.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <sys/param.h>
 
diff --git a/lib/private.h b/lib/private.h
index 49f9152..fde5e3c 100644
--- a/lib/private.h
+++ b/lib/private.h
@@ -33,8 +33,10 @@
 #ifndef LIB_VFIO_USER_PRIVATE_H
 #define LIB_VFIO_USER_PRIVATE_H
 
+#include <errno.h>
+
 #include "pci_caps.h"
-#include "dma.h"
+#include "common.h"
 
 static inline int
 ERROR_INT(int err)
@@ -109,9 +111,11 @@ struct pci_dev {
     size_t                  nr_ext_caps;
 };
 
+struct dma_controller;
+
 struct vfu_ctx {
     void                    *pvt;
-    dma_controller_t        *dma;
+    struct dma_controller   *dma;
     vfu_reset_cb_t          *reset;
     int           log_level;
     vfu_log_fn_t            *log;
diff --git a/lib/tran_sock.c b/lib/tran_sock.c
index ce3430a..a1d6a1a 100644
--- a/lib/tran_sock.c
+++ b/lib/tran_sock.c
@@ -30,6 +30,7 @@
  *
  */
 
+#include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <limits.h>