MINOR: fd: pass the iocb and owner to fd_insert()

fd_insert() is currently called just after setting the owner and iocb,
but proceeding like this prevents the operation from being atomic and
requires a lock to protect the maxfd computation in another thread from
meeting an incompletely initialized FD and computing a wrong maxfd.
Fortunately for now all fdtab[].owner are set before calling fd_insert(),
and the first lock in fd_insert() enforces a memory barrier so the code
is safe.

This patch moves the initialization of the owner and iocb to fd_insert()
so that the function will be able to properly arrange its operations and
remain safe even when modified to become lockless. There's no other change
beyond the internal API.

diff --git a/include/proto/connection.h b/include/proto/connection.h
index 64117641c505e8a46ca3704a61b1905cc4837fcb..bc8d88484c14ceedff398d99680548e7be9aadb0 100644 (file)
--- a/include/proto/connection.h
+++ b/include/proto/connection.h
@@ -111,9 +111,7 @@ static inline void conn_ctrl_init(struct connection *conn)
        if (!conn_ctrl_ready(conn)) {
                int fd = conn->handle.fd;
 
-               fdtab[fd].owner = conn;
-               fdtab[fd].iocb = conn_fd_handler;
-               fd_insert(fd, tid_bit);
+               fd_insert(fd, conn, conn_fd_handler, tid_bit);
                /* mark the fd as ready so as not to needlessly poll at the beginning */
                fd_may_recv(fd);
                fd_may_send(fd);

diff --git a/include/proto/fd.h b/include/proto/fd.h
index 0dd3e84a8d232a9c4e66b1afde0110415c040d00..a7e70b7fd4655822c02faddcab5a686be2330edf 100644 (file)
--- a/include/proto/fd.h
+++ b/include/proto/fd.h
@@ -393,9 +393,11 @@ static inline void fd_update_events(int fd, int evts)
 }
 
 /* Prepares <fd> for being polled */
-static inline void fd_insert(int fd, unsigned long thread_mask)
+static inline void fd_insert(int fd, void *owner, void (*iocb)(int fd), unsigned long thread_mask)
 {
        HA_SPIN_LOCK(FD_LOCK, &fdtab[fd].lock);
+       fdtab[fd].owner = owner;
+       fdtab[fd].iocb = iocb;
        fdtab[fd].ev = 0;
        fdtab[fd].update_mask &= ~tid_bit;
        fdtab[fd].linger_risk = 0;

diff --git a/src/dns.c b/src/dns.c
index a957710edb2f92e31069844a15338cc8834e8402..280bc155f78cef09a8cc5052c7e6d8a213905012 100644 (file)
--- a/src/dns.c
+++ b/src/dns.c
@@ -193,9 +193,7 @@ static int dns_connect_namesaver(struct dns_nameserver *ns)
 
        /* Add the fd in the fd list and update its parameters */
        dgram->t.sock.fd = fd;
-       fdtab[fd].owner  = dgram;
-       fdtab[fd].iocb   = dgram_fd_handler;
-       fd_insert(fd, (unsigned long)-1);
+       fd_insert(fd, dgram, dgram_fd_handler, MAX_THREADS_MASK);
        fd_want_recv(fd);
        return 0;
 }

diff --git a/src/haproxy.c b/src/haproxy.c
index 11849fe22f96769e432f39441de86b9bace46437..f1a2fb9d48190c5a5f441e80ee2beb82b47f7de3 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2347,9 +2347,7 @@ void mworker_pipe_register()
                return;
 
        fcntl(mworker_pipe[0], F_SETFL, O_NONBLOCK);
-       fdtab[mworker_pipe[0]].owner = mworker_pipe;
-       fdtab[mworker_pipe[0]].iocb = mworker_pipe_handler;
-       fd_insert(mworker_pipe[0], MAX_THREADS_MASK);
+       fd_insert(mworker_pipe[0], mworker_pipe, mworker_pipe_handler, MAX_THREADS_MASK);
        fd_want_recv(mworker_pipe[0]);
 }
 

diff --git a/src/hathreads.c b/src/hathreads.c
index fea4ffeb0b959479296dfe5ee1afcadd3a79035c..fd0eb68464721f2997170502112345b9bb5fbb7c 100644 (file)
--- a/src/hathreads.c
+++ b/src/hathreads.c
@@ -48,10 +48,7 @@ int thread_sync_init(unsigned long mask)
 
        rfd = threads_sync_pipe[0];
        fcntl(rfd, F_SETFL, O_NONBLOCK);
-
-       fdtab[rfd].owner = thread_sync_io_handler;
-       fdtab[rfd].iocb = thread_sync_io_handler;
-       fd_insert(rfd, MAX_THREADS_MASK);
+       fd_insert(rfd, thread_sync_io_handler, thread_sync_io_handler, MAX_THREADS_MASK);
 
        all_threads_mask = mask;
        return 0;

diff --git a/src/proto_tcp.c b/src/proto_tcp.c
index f4ff698fd542dd97cbe9177a774037278fcb3803..9a3689c719685dd8424066b1902d41448bc9f798 100644 (file)
--- a/src/proto_tcp.c
+++ b/src/proto_tcp.c
@@ -1105,12 +1105,9 @@ int tcp_bind_listener(struct listener *listener, char *errmsg, int errlen)
        listener->fd = fd;
        listener->state = LI_LISTEN;
 
-       fdtab[fd].owner = listener; /* reference the listener instead of a task */
-       fdtab[fd].iocb = listener->proto->accept;
-       if (listener->bind_conf->bind_thread[relative_pid-1])
-               fd_insert(fd, listener->bind_conf->bind_thread[relative_pid-1]);
-       else
-               fd_insert(fd, MAX_THREADS_MASK);
+       fd_insert(fd, listener, listener->proto->accept,
+                 listener->bind_conf->bind_thread[relative_pid-1] ?
+                 listener->bind_conf->bind_thread[relative_pid-1] : MAX_THREADS_MASK);
 
  tcp_return:
        if (msg && errlen) {

diff --git a/src/proto_uxst.c b/src/proto_uxst.c
index 754acd2c7dfc95aa0365ce48d1117cac9bce1176..3ab637f20d19b692be3e323906c47f5e008b153f 100644 (file)
--- a/src/proto_uxst.c
+++ b/src/proto_uxst.c
@@ -331,13 +331,10 @@ static int uxst_bind_listener(struct listener *listener, char *errmsg, int errle
        listener->fd = fd;
        listener->state = LI_LISTEN;
 
-       /* the function for the accept() event */
-       fdtab[fd].iocb = listener->proto->accept;
-       fdtab[fd].owner = listener; /* reference the listener instead of a task */
-       if (listener->bind_conf->bind_thread[relative_pid-1])
-               fd_insert(fd, listener->bind_conf->bind_thread[relative_pid-1]);
-       else
-               fd_insert(fd, MAX_THREADS_MASK);
+       fd_insert(fd, listener, listener->proto->accept,
+                 listener->bind_conf->bind_thread[relative_pid-1] ?
+                 listener->bind_conf->bind_thread[relative_pid-1] : MAX_THREADS_MASK);
+
        return err;
 
  err_rename:

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index aecf3ddb7c57f55da25d85ae231d5db8262053ee..c2fa45f52df71cccb4889291554b0c6b6c52d264 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -488,7 +488,7 @@ static void ssl_async_fd_free(int fd)
  */
 static void inline ssl_async_process_fds(struct connection *conn, SSL *ssl)
 {
-       OSSL_ASYNC_FD add_fd[32], afd;
+       OSSL_ASYNC_FD add_fd[32];
        OSSL_ASYNC_FD del_fd[32];
        size_t num_add_fds = 0;
        size_t num_del_fds = 0;
@@ -509,10 +509,7 @@ static void inline ssl_async_process_fds(struct connection *conn, SSL *ssl)
 
        /* We add new fds to the fdtab */
        for (i=0 ; i < num_add_fds ; i++) {
-               afd = add_fd[i];
-               fdtab[afd].owner = conn;
-               fdtab[afd].iocb = ssl_async_fd_handler;
-               fd_insert(afd, tid_bit);
+               fd_insert(add_fd[i], conn, ssl_async_fd_handler, tid_bit);
        }
 
        num_add_fds = 0;