make file descriptors private to the transport (#321)

General code has no business knowing about the socket file descriptors.
vfu_attach_ctx() is changed to not return the file descriptor; we'll re-expose a
suitable file descriptor in a follow-up

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

5 files changed, 121 insertions, 46 deletions

diff --git a/include/libvfio-user.h b/include/libvfio-user.h
index e48514a..11666d0 100644
--- a/include/libvfio-user.h
+++ b/include/libvfio-user.h
@@ -116,12 +116,13 @@ int
 vfu_realize_ctx(vfu_ctx_t *vfu_ctx);
 
 /*
- * Attempts to attach to the transport. Attach is mandatory before
- * vfu_run_ctx() and is non blocking if context is created
- * with LIBVFIO_USER_FLAG_ATTACH_NB flag.
- * Returns client's file descriptor on success and -1 on error. If errno is
- * set to EAGAIN or EWOULDBLOCK then the transport is not ready to attach to and
- * the operation must be retried.
+ * Attempts to attach to the transport. Attach is mandatory before vfu_run_ctx()
+ * and is non blocking if context is created with LIBVFIO_USER_FLAG_ATTACH_NB
+ * flag.
+ *
+ * @returns: 0 on success, -1 on error. Sets errno.  If errno is set to EAGAIN
+ * or EWOULDBLOCK then the transport is not ready to attach to and the operation
+ * must be retried.
  *
  * @vfu_ctx: the libvfio-user context
  */
diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index c36a4da..738d3b7 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -1143,10 +1143,9 @@ vfu_create_ctx(vfu_trans_t trans, const char *path, int flags, void *pvt,
         return ERROR_PTR(ENOMEM);
     }
 
-    vfu_ctx->fd = -1;
-    vfu_ctx->conn_fd = -1;
     vfu_ctx->dev_type = dev_type;
     vfu_ctx->tran = &tran_sock_ops;
+    vfu_ctx->tran_data = NULL;
     vfu_ctx->pvt = pvt;
     vfu_ctx->flags = flags;
     vfu_ctx->log_level = LOG_ERR;
@@ -1183,7 +1182,6 @@ vfu_create_ctx(vfu_trans_t trans, const char *path, int flags, void *pvt,
         if (err < 0) {
             goto err_out;
         }
-        vfu_ctx->fd = err;
     }
 
     for (i = 0; i< vfu_ctx->nr_regions; i++) {
diff --git a/lib/private.h b/lib/private.h
index c38cc09..5ea25be 100644
--- a/lib/private.h
+++ b/lib/private.h
@@ -117,8 +117,6 @@ struct pci_dev {
 struct vfu_ctx {
     void                    *pvt;
     dma_controller_t        *dma;
-    int                     fd;
-    int                     conn_fd;
     vfu_reset_cb_t          *reset;
     int           log_level;
     vfu_log_fn_t            *log;
@@ -126,6 +124,7 @@ struct vfu_ctx {
     vfu_reg_info_t          *reg_info;
     struct pci_dev          pci;
     struct transport_ops    *tran;
+    void                    *tran_data;
     uint64_t                flags;
     char                    *uuid;
     vfu_map_dma_cb_t        *map_dma;
diff --git a/lib/tran_sock.c b/lib/tran_sock.c
index 4762c71..4aa4ee0 100644
--- a/lib/tran_sock.c
+++ b/lib/tran_sock.c
@@ -51,26 +51,42 @@
 // FIXME: is this the value we want?
 #define SERVER_MAX_FDS 8
 
+typedef struct {
+    int listen_fd;
+    int conn_fd;
+} tran_sock_t;
+
 static int
 tran_sock_init(vfu_ctx_t *vfu_ctx)
 {
     struct sockaddr_un addr = { .sun_family = AF_UNIX };
-    int ret, unix_sock;
+    tran_sock_t *ts = NULL;
     mode_t mode;
+    int ret;
 
     assert(vfu_ctx != NULL);
 
     /* FIXME SPDK can't easily run as non-root */
-    mode =  umask(0000);
+    mode = umask(0000);
+
+    ts = calloc(1, sizeof(tran_sock_t));
+
+    if (ts == NULL) {
+        ret = -errno;
+        goto out;
+    }
 
-    if ((unix_sock = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
+    ts->listen_fd = -1;
+    ts->conn_fd = -1;
+
+    if ((ts->listen_fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
 	    ret = -errno;
         goto out;
     }
 
     if (vfu_ctx->flags & LIBVFIO_USER_FLAG_ATTACH_NB) {
-        ret = fcntl(unix_sock, F_SETFL,
-                    fcntl(unix_sock, F_GETFL, 0) | O_NONBLOCK);
+        ret = fcntl(ts->listen_fd, F_SETFL,
+                    fcntl(ts->listen_fd, F_GETFL, 0) | O_NONBLOCK);
         if (ret < 0) {
             ret = -errno;
             goto out;
@@ -78,7 +94,7 @@ tran_sock_init(vfu_ctx_t *vfu_ctx)
     }
 
     ret = snprintf(addr.sun_path, sizeof addr.sun_path, "%s", vfu_ctx->uuid);
-    if (ret >= (int)sizeof addr.sun_path) {
+    if (ret >= (int)sizeof(addr.sun_path)) {
         ret = -ENAMETOOLONG;
     }
     if (ret < 0) {
@@ -86,26 +102,30 @@ tran_sock_init(vfu_ctx_t *vfu_ctx)
     }
 
     /* start listening business */
-    ret = bind(unix_sock, (struct sockaddr*)&addr, sizeof(addr));
+    ret = bind(ts->listen_fd, (struct sockaddr *)&addr, sizeof(addr));
     if (ret < 0) {
 	    ret = -errno;
         goto out;
     }
 
-    ret = listen(unix_sock, 0);
+    ret = listen(ts->listen_fd, 0);
     if (ret < 0) {
         ret = -errno;
     }
 
 out:
     umask(mode);
+
     if (ret != 0) {
-        if (unix_sock >= 0) {
-            close(unix_sock);
+        if (ts->listen_fd != -1) {
+            close(ts->listen_fd);
         }
+        free(ts);
         return ret;
     }
-    return unix_sock;
+
+    vfu_ctx->tran_data = ts;
+    return 0;
 }
 
 int
@@ -675,32 +695,41 @@ negotiate(vfu_ctx_t *vfu_ctx, int sock)
 static int
 tran_sock_attach(vfu_ctx_t *vfu_ctx)
 {
+    tran_sock_t *ts;
     int ret;
-    int conn_fd;
 
     assert(vfu_ctx != NULL);
+    assert(vfu_ctx->tran_data != NULL);
 
-    conn_fd = accept(vfu_ctx->fd, NULL, NULL);
-    if (conn_fd == -1) {
-        return conn_fd;
+    ts = vfu_ctx->tran_data;
+
+    ts->conn_fd = accept(ts->listen_fd, NULL, NULL);
+    if (ts->conn_fd == -1) {
+        return -errno;
     }
 
-    ret = negotiate(vfu_ctx, conn_fd);
+    ret = negotiate(vfu_ctx, ts->conn_fd);
     if (ret < 0) {
-        close(conn_fd);
+        close(ts->conn_fd);
+        ts->conn_fd = -1;
         return ret;
     }
 
-    vfu_ctx->conn_fd = conn_fd;
-    return conn_fd;
+    return 0;
 }
 
 static int
 tran_sock_get_request(vfu_ctx_t *vfu_ctx, struct vfio_user_header *hdr,
                       int *fds, size_t *nr_fds)
 {
+    tran_sock_t *ts;
     int sock_flags = 0;
 
+    assert(vfu_ctx != NULL);
+    assert(vfu_ctx->tran_data != NULL);
+
+    ts = vfu_ctx->tran_data;
+
     /*
      * TODO ideally we should set O_NONBLOCK on the fd so that the syscall is
      * faster (?). I tried that and get short reads, so we need to store the
@@ -709,17 +738,25 @@ tran_sock_get_request(vfu_ctx_t *vfu_ctx, struct vfio_user_header *hdr,
     if (vfu_ctx->flags & LIBVFIO_USER_FLAG_ATTACH_NB) {
         sock_flags = MSG_DONTWAIT | MSG_WAITALL;
     }
-    return get_msg(hdr, sizeof *hdr, fds, nr_fds, vfu_ctx->conn_fd, sock_flags);
+    return get_msg(hdr, sizeof *hdr, fds, nr_fds, ts->conn_fd, sock_flags);
 }
 
 static int
 tran_sock_recv_body(vfu_ctx_t *vfu_ctx, const struct vfio_user_header *hdr,
                     void **datap)
 {
-    size_t body_size = hdr->msg_size - sizeof (*hdr);
+    size_t body_size;
+    tran_sock_t *ts;
     void *data;
     int ret;
 
+    assert(vfu_ctx != NULL);
+    assert(vfu_ctx->tran_data != NULL);
+    assert(hdr != NULL);
+
+    ts = vfu_ctx->tran_data;
+
+    body_size = hdr->msg_size - sizeof (*hdr);
     // FIXME: should check max-msg-size
     data = malloc(body_size);
 
@@ -727,7 +764,8 @@ tran_sock_recv_body(vfu_ctx_t *vfu_ctx, const struct vfio_user_header *hdr,
         return -errno;
     }
 
-    ret = recv(vfu_ctx->conn_fd, data, body_size, 0);
+    ret = recv(ts->conn_fd, data, body_size, 0);
+
     if (ret < 0) {
         ret = -errno;
         free(data);
@@ -750,8 +788,15 @@ tran_sock_reply(vfu_ctx_t *vfu_ctx, uint16_t msg_id,
                 struct iovec *iovecs, size_t nr_iovecs,
                 int *fds, int count, int err)
 {
+    tran_sock_t *ts;
+
+    assert(vfu_ctx != NULL);
+    assert(vfu_ctx->tran_data != NULL);
+
+    ts = vfu_ctx->tran_data;
+
     // FIXME: SPEC: should the reply include the command? I'd say yes?
-    return tran_sock_send_iovec(vfu_ctx->conn_fd, msg_id, true, 0,
+    return tran_sock_send_iovec(ts->conn_fd, msg_id, true, 0,
                                 iovecs, nr_iovecs, fds, count, err);
 }
 
@@ -762,26 +807,52 @@ tran_sock_send_msg(vfu_ctx_t *vfu_ctx, uint16_t msg_id,
               struct vfio_user_header *hdr,
               void *recv_data, size_t recv_len)
 {
-    return tran_sock_msg(vfu_ctx->conn_fd, msg_id, cmd, send_data, send_len,
+    tran_sock_t *ts;
+
+    assert(vfu_ctx != NULL);
+    assert(vfu_ctx->tran_data != NULL);
+
+    ts = vfu_ctx->tran_data;
+
+    return tran_sock_msg(ts->conn_fd, msg_id, cmd, send_data, send_len,
                          hdr, recv_data, recv_len);
 }
 
 static void
 tran_sock_detach(vfu_ctx_t *vfu_ctx)
 {
-    if (vfu_ctx->conn_fd != -1) {
-        (void) close(vfu_ctx->conn_fd);
-        vfu_ctx->conn_fd = -1;
+    tran_sock_t *ts;
+
+    assert(vfu_ctx != NULL);
+    assert(vfu_ctx->tran_data != NULL);
+
+    ts = vfu_ctx->tran_data;
+
+    if (ts->conn_fd != -1) {
+        // FIXME: handle EINTR
+        (void) close(ts->conn_fd);
+        ts->conn_fd = -1;
     }
 }
 
 static void
 tran_sock_fini(vfu_ctx_t *vfu_ctx)
 {
-    if (vfu_ctx->fd != -1) {
-        (void) close(vfu_ctx->fd);
-        vfu_ctx->fd = -1;
+    tran_sock_t *ts;
+
+    assert(vfu_ctx != NULL);
+    assert(vfu_ctx->tran_data != NULL);
+
+    ts = vfu_ctx->tran_data;
+
+    if (ts->listen_fd != -1) {
+        // FIXME: handle EINTR
+        (void) close(ts->listen_fd);
+        ts->listen_fd = -1;
     }
+
+    free(vfu_ctx->tran_data);
+    vfu_ctx->tran_data = NULL;
 }
 
 struct transport_ops tran_sock_ops = {
diff --git a/test/unit-tests.c b/test/unit-tests.c
index 738bae5..1fdd241 100644
--- a/test/unit-tests.c
+++ b/test/unit-tests.c
@@ -337,6 +337,11 @@ set_nr_fds(const long unsigned int value,
     return 1;
 }
 
+typedef struct {
+    int fd;
+    int conn_fd;
+} tran_sock_t;
+
 /*
  * Tests that if if exec_command fails then process_request frees passed file
  * descriptors.
@@ -344,11 +349,12 @@ set_nr_fds(const long unsigned int value,
 static void
 test_process_command_free_passed_fds(void **state __attribute__((unused)))
 {
+    tran_sock_t ts = { .fd = 23, .conn_fd = 24 };
     vfu_ctx_t vfu_ctx = {
-        .conn_fd = 0xcafebabe,
         .client_max_fds = ARRAY_SIZE(fds),
         .migration = (struct migration *)0x8badf00d,
         .tran = &tran_sock_ops,
+        .tran_data = &ts
     };
 
     patch(get_next_command);
@@ -377,7 +383,7 @@ test_process_command_free_passed_fds(void **state __attribute__((unused)))
     will_return(__wrap_close, 0);
 
     patch(tran_sock_send_iovec);
-    expect_value(__wrap_tran_sock_send_iovec, sock, vfu_ctx.conn_fd);
+    expect_value(__wrap_tran_sock_send_iovec, sock, ts.conn_fd);
     expect_any(__wrap_tran_sock_send_iovec, msg_id);
     expect_value(__wrap_tran_sock_send_iovec, is_reply, true);
     expect_any(__wrap_tran_sock_send_iovec, cmd);
@@ -420,7 +426,7 @@ dummy_attach(vfu_ctx_t *vfu_ctx)
 {
     assert(vfu_ctx != NULL);
 
-    return 222;
+    return 0;
 }
 
 static void
@@ -433,7 +439,7 @@ test_attach_ctx(void **state __attribute__((unused)))
         .tran = &transport_ops,
     };
 
-    assert_int_equal(222, vfu_attach_ctx(&vfu_ctx));
+    assert_int_equal(0, vfu_attach_ctx(&vfu_ctx));
 }
 
 static void
@@ -603,7 +609,7 @@ test_vfu_ctx_create(void **state __attribute__((unused)))
     assert_int_equal(0, vfu_realize_ctx(vfu_ctx));
 
     patch(close);
-    expect_value(__wrap_close, fd, vfu_ctx->fd);
+    expect_value(__wrap_close, fd, ((tran_sock_t *)vfu_ctx->tran_data)->fd);
     will_return(__wrap_close, 0);
 
     vfu_destroy_ctx(vfu_ctx);