Introduce close_safely helper function (#763)
The helper function centralizes some extra checks and diligence desired
by many/most current code paths but currently inconsistently applied.
This includes bypassing the close call when the file descriptor is -1
already, resetting the file descriptor variable to -1 after closing, and
preserving errno.
All calls to close are replaced by close_safely. Some warning log output
is lost over this, but it doesn't seem like this was very useful anyways
given that Linux always closes the file descriptor anyways.
Signed-off-by: Mattias Nissler <mnissler@rivosinc.com>
diff --git a/lib/common.h b/lib/common.h
index 0c3abda..07a74a5 100644
--- a/lib/common.h
+++ b/lib/common.h
@@ -37,8 +37,10 @@
#ifndef LIB_VFIO_USER_COMMON_H
#define LIB_VFIO_USER_COMMON_H
+#include <errno.h>
#include <limits.h>
#include <stdint.h>
+#include <unistd.h>
#define UNUSED __attribute__((unused))
#define EXPORT __attribute__((visibility("default")))
@@ -79,6 +81,32 @@
return ROUND_UP(nr_pages, sizeof(uint64_t) * CHAR_BIT) / CHAR_BIT;
}
+/*
+ * Closes the given file descriptor, and resets the value to -1. Preserves
+ * errno. Skips closing if *fd is -1.
+ */
+static inline void
+close_safely(int *fd)
+{
+ int saved_errno = errno;
+ if (fd != NULL && *fd != -1) {
+ /*
+ * POSIX says that close may hit EINTR and leave the file descriptor in
+ * undefined state. But retrying on EINTR is incorrect, since a
+ * different thread might have re-opened a file on the same descriptor
+ * if close actually did free the descriptor. In practice, Linux always
+ * closes the file descriptor and POSIX has decided to align semantics
+ * with Linux. Thus, calling close once and ignoring the error is the
+ * most appropriate course of action.
+ *
+ * See also https://www.austingroupbugs.net/view.php?id=529
+ */
+ (void) close(*fd);
+ *fd = -1;
+ }
+ errno = saved_errno;
+}
+
#ifdef UNIT_TEST
#define MOCK_DEFINE(f) \
diff --git a/lib/dma.c b/lib/dma.c
index beefeac..af1495f 100644
--- a/lib/dma.c
+++ b/lib/dma.c
@@ -121,10 +121,7 @@
assert(region->fd != -1);
- if (close(region->fd) == -1) {
- vfu_log(dma->vfu_ctx, LOG_WARNING, "failed to close fd %d: %m",
- region->fd);
- }
+ close_safely(®ion->fd);
}
static void
@@ -402,10 +399,7 @@
vfu_log(dma->vfu_ctx, LOG_ERR,
"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);
- }
+ close_safely(®ion->fd);
free(region->dirty_bitmap);
return ERROR_INT(ret);
}
diff --git a/lib/irq.c b/lib/irq.c
index 8a5d200..183d071 100644
--- a/lib/irq.c
+++ b/lib/irq.c
@@ -122,14 +122,7 @@
}
for (i = start; i < count; i++) {
- if (efds[i] >= 0) {
- if (close(efds[i]) == -1) {
- vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m",
- efds[i]);
- }
-
- efds[i] = -1;
- }
+ close_safely(&efds[i]);
}
}
@@ -143,14 +136,7 @@
irqs_disable(vfu_ctx, VFIO_PCI_ERR_IRQ_INDEX, 0, 0);
for (i = 0; i < vfu_ctx->irqs->max_ivs; i++) {
- if (efds[i] >= 0) {
- if (close(efds[i]) == -1) {
- vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m",
- efds[i]);
- }
-
- efds[i] = -1;
- }
+ close_safely(&efds[i]);
}
}
@@ -257,12 +243,7 @@
for (i = irq_set->start, j = 0; i < (irq_set->start + irq_set->count);
i++, j++) {
efd = irqs_get_efd(vfu_ctx, irq_set->index, i);
- if (*efd >= 0) {
- if (close(*efd) == -1) {
- vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m", *efd);
- }
- *efd = -1;
- }
+ close_safely(efd);
assert(data[j] >= 0);
/*
* We've already checked in handle_device_set_irqs that
diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index 663d2cd..48013c2 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -755,12 +755,9 @@
dma_map->size, fd, dma_map->offset,
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);
- }
- return ERROR_INT(ret);
+ close_safely(&fd);
+ return -1;
}
if (vfu_ctx->dma_register != NULL) {
@@ -1096,14 +1093,12 @@
free(msg->in.iov.iov_base);
for (i = 0; i < msg->in.nr_fds; i++) {
- if (msg->in.fds[i] != -1) {
- if (msg->processed_cmd) {
- vfu_log(vfu_ctx, LOG_DEBUG,
- "closing unexpected fd %d (index %zu) from cmd %u",
- msg->in.fds[i], i, msg->hdr.cmd);
- }
- close(msg->in.fds[i]);
+ if (msg->in.fds[i] != -1 && msg->processed_cmd) {
+ vfu_log(vfu_ctx, LOG_DEBUG,
+ "closing unexpected fd %d (index %zu) from cmd %u",
+ msg->in.fds[i], i, msg->hdr.cmd);
}
+ close_safely(&msg->in.fds[i]);
}
free(msg->in.fds);
@@ -1276,11 +1271,9 @@
*msgp = alloc_msg(&hdr, fds, nr_fds);
if (*msgp == NULL) {
- int saved_errno = errno;
for (i = 0; i < nr_fds; i++) {
- close(fds[i]);
+ close_safely(&fds[i]);
}
- errno = saved_errno;
return -1;
}
diff --git a/lib/tran.c b/lib/tran.c
index a183877..dff47ef 100644
--- a/lib/tran.c
+++ b/lib/tran.c
@@ -252,9 +252,7 @@
(void) vfu_ctx->tran->reply(vfu_ctx, &rmsg, ret);
for (i = 0; i < msg.in.nr_fds; i++) {
- if (msg.in.fds[i] != -1) {
- close(msg.in.fds[i]);
- }
+ close_safely(&msg.in.fds[i]);
}
free(msg.in.iov.iov_base);
diff --git a/lib/tran_sock.c b/lib/tran_sock.c
index 7c4e30b..fb0ccbb 100644
--- a/lib/tran_sock.c
+++ b/lib/tran_sock.c
@@ -419,8 +419,8 @@
out:
if (ret != 0) {
- if (ts != NULL && ts->listen_fd != -1) {
- close(ts->listen_fd);
+ if (ts != NULL) {
+ close_safely(&ts->listen_fd);
}
free(ts);
return ERROR_INT(ret);
@@ -466,10 +466,8 @@
ret = tran_negotiate(vfu_ctx);
if (ret < 0) {
- ret = errno;
- close(ts->conn_fd);
- ts->conn_fd = -1;
- return ERROR_INT(ret);
+ close_safely(&ts->conn_fd);
+ return -1;
}
return 0;
@@ -636,10 +634,8 @@
ts = vfu_ctx->tran_data;
- if (ts != NULL && ts->conn_fd != -1) {
- // FIXME: handle EINTR
- (void) close(ts->conn_fd);
- ts->conn_fd = -1;
+ if (ts != NULL) {
+ close_safely(&ts->conn_fd);
}
}
@@ -654,11 +650,7 @@
if (ts != NULL) {
(void) unlink(vfu_ctx->uuid);
- if (ts->listen_fd != -1) {
- // FIXME: handle EINTR
- (void) close(ts->listen_fd);
- ts->listen_fd = -1;
- }
+ close_safely(&ts->listen_fd);
}
free(vfu_ctx->tran_data);
