From 451b4180c18722b11047ba6c7554e24dde0004f9 Mon Sep 17 00:00:00 2001 From: John Levon Date: Tue, 13 Apr 2021 10:35:59 +0100 Subject: 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 Reviewed-by: Thanos Makatos --- lib/dma.c | 78 ++++++++++++++++++++++++------------------------------ lib/dma.h | 13 +++++---- lib/irq.c | 2 ++ lib/libvfio-user.c | 19 ++++++++----- lib/pci.c | 1 + lib/private.h | 8 ++++-- lib/tran_sock.c | 1 + 7 files changed, 64 insertions(+), 58 deletions(-) (limited to 'lib') 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, ®ion->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(®ion->info.iova), - strerror(err)); - return -err; + "failed to dma_unregister() DMA region [%p, %p): %m", + region->info.iova.iov_base, iov_end(®ion->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(®ion->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 #include #include +#include #include #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 #include #include +#include #include #include 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 + #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 #include #include #include -- cgit v1.1