Introduce close_safely helper function

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>

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 @@ _get_bitmap_size(size_t size, size_t pgsize)
  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) \

lib/dma.c

@@ -121,10 +121,7 @@ MOCK_DEFINE(dma_controller_unmap_region)(dma_controller_t *dma,
 
  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(&region->fd);
 }
 
 static void
@@ -402,10 +399,7 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
  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(&region->fd);
  free(region->dirty_bitmap);
  return ERROR_INT(ret);
  }

lib/irq.c

@@ -122,14 +122,7 @@ irqs_disable(vfu_ctx_t *vfu_ctx, uint32_t index, uint32_t start, uint32_t count)
  }
 
  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_reset(vfu_ctx_t *vfu_ctx)
  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 @@ irqs_set_data_eventfd(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set,
  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

lib/libvfio-user.c

@@ -755,12 +755,9 @@ handle_dma_map(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
  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(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
  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 @@ get_request_header(vfu_ctx_t *vfu_ctx, vfu_msg_t **msgp)
  *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;
  }
 

lib/tran.c

@@ -252,9 +252,7 @@ recv_version(vfu_ctx_t *vfu_ctx, uint16_t *msg_idp,
  (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);

lib/tran_sock.c

@@ -419,8 +419,8 @@ tran_sock_init(vfu_ctx_t *vfu_ctx)
 
 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 @@ tran_sock_attach(vfu_ctx_t *vfu_ctx)
 
  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 @@ tran_sock_detach(vfu_ctx_t *vfu_ctx)
 
  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 @@ tran_sock_fini(vfu_ctx_t *vfu_ctx)
 
  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);