Clean up "close socket" routines.
authorAlan T. DeKok <aland@freeradius.org>
Tue, 20 Aug 2013 14:21:37 +0000 (10:21 -0400)
committerAlan T. DeKok <aland@freeradius.org>
Tue, 20 Aug 2013 14:24:08 +0000 (10:24 -0400)
We use the new rbtree DeleteOrder when walking over the packet list.
When the "eol tcp/proxy listener" callback is run, it can return
"please delete this node", instead of calling rbtree_delete.

Doing that allows the walker to be aware of deletions, unlike
before.

This turned out to make the code simpler, and the whole event fd
handling has become more robust.

src/include/packet.h
src/include/radiusd.h
src/lib/packet.c
src/main/command.c
src/main/listen.c
src/main/process.c
src/main/radclient.c
src/main/tls_listen.c

index 7574f26..d831814 100644 (file)
@@ -54,12 +54,11 @@ int fr_packet_list_num_elements(fr_packet_list_t *pl);
 bool fr_packet_list_id_alloc(fr_packet_list_t *pl, int proto,
                            RADIUS_PACKET **request_p, void **pctx);
 bool fr_packet_list_id_free(fr_packet_list_t *pl,
-                            RADIUS_PACKET *request);
+                           RADIUS_PACKET *request, bool yank);
 bool fr_packet_list_socket_add(fr_packet_list_t *pl, int sockfd, int proto,
                              fr_ipaddr_t *dst_ipaddr, int dst_port,
                              void *ctx);
-bool fr_packet_list_socket_del(fr_packet_list_t *pl, int sockfd,
-                                void **pctx);
+bool fr_packet_list_socket_del(fr_packet_list_t *pl, int sockfd);
 bool fr_packet_list_socket_freeze(fr_packet_list_t *pl, int sockfd);
 bool fr_packet_list_socket_thaw(fr_packet_list_t *pl, int sockfd);
 int fr_packet_list_walk(fr_packet_list_t *pl, void *ctx,
index 4360271..78a5b35 100644 (file)
@@ -413,11 +413,10 @@ typedef struct listen_socket_t {
        RADCLIENT_LIST  *clients;
 } listen_socket_t;
 
-#define RAD_LISTEN_STATUS_INIT   (0)
-#define RAD_LISTEN_STATUS_KNOWN  (1)
-#define RAD_LISTEN_STATUS_REMOVE_FD (2)
-#define RAD_LISTEN_STATUS_CLOSED (3)
-#define RAD_LISTEN_STATUS_FINISH (4)
+#define RAD_LISTEN_STATUS_INIT       (0)
+#define RAD_LISTEN_STATUS_KNOWN      (1)
+#define RAD_LISTEN_STATUS_EOL       (2)
+#define RAD_LISTEN_STATUS_REMOVE_NOW (3)
 
 typedef struct main_config_t {
        struct main_config *next;
index e4a44db..4a8e6ea 100644 (file)
@@ -327,8 +327,8 @@ bool fr_packet_list_socket_thaw(fr_packet_list_t *pl, int sockfd)
        return true;
 }
 
-bool fr_packet_list_socket_del(fr_packet_list_t *pl, int sockfd,
-                                void **pctx)
+
+bool fr_packet_list_socket_del(fr_packet_list_t *pl, int sockfd)
 {
        fr_packet_socket_t *ps;
 
@@ -337,14 +337,10 @@ bool fr_packet_list_socket_del(fr_packet_list_t *pl, int sockfd,
        ps = fr_socket_find(pl, sockfd);
        if (!ps) return false;
 
-       /*
-        *      FIXME: Allow the caller forcibly discard these?
-        */
        if (ps->num_outgoing != 0) return false;
 
        ps->sockfd = -1;
        pl->num_sockets--;
-       if (pctx) *pctx = ps->ctx;
 
        return true;
 }
@@ -784,7 +780,7 @@ bool fr_packet_list_id_alloc(fr_packet_list_t *pl, int proto,
  *     any newly inserted entries don't collide with this one.
  */
 bool fr_packet_list_id_free(fr_packet_list_t *pl,
-                            RADIUS_PACKET *request)
+                           RADIUS_PACKET *request, bool yank)
 {
        fr_packet_socket_t *ps;
 
@@ -792,7 +788,7 @@ bool fr_packet_list_id_free(fr_packet_list_t *pl,
 
        VERIFY_PACKET(request);
 
-       if (!fr_packet_list_yank(pl, request)) return false;
+       if (yank && !fr_packet_list_yank(pl, request)) return false;
 
        ps = fr_socket_find(pl, request->sockfd);
        if (!ps) return false;
@@ -809,12 +805,19 @@ bool fr_packet_list_id_free(fr_packet_list_t *pl,
        return true;
 }
 
+/*
+ *     We always walk DeleteOrder, which is like InOrder, except that
+ *     <0 means error, stop
+ *     0  means OK, continue
+ *     1  means delete current node and stop
+ *     2  means delete current node and continue
+ */
 int fr_packet_list_walk(fr_packet_list_t *pl, void *ctx,
-                         fr_hash_table_walk_t callback)
+                       fr_hash_table_walk_t callback)
 {
        if (!pl || !callback) return 0;
 
-       return rbtree_walk(pl->tree, InOrder, callback, ctx);
+       return rbtree_walk(pl->tree, DeleteOrder, callback, ctx);
 }
 
 int fr_packet_list_fd_set(fr_packet_list_t *pl, fd_set *set)
index 8b60f6f..1ac165f 100644 (file)
@@ -273,7 +273,7 @@ static int fr_server_domain_socket(char const *path)
 
 static void command_close_socket(rad_listen_t *this)
 {
-       this->status = RAD_LISTEN_STATUS_CLOSED;
+       this->status = RAD_LISTEN_STATUS_EOL;
 
        /*
         *      This removes the socket from the event fd, so no one
@@ -293,7 +293,7 @@ static ssize_t cprintf(rad_listen_t *listener, char const *fmt, ...)
        len = vsnprintf(buffer, sizeof(buffer), fmt, ap);
        va_end(ap);
 
-       if (listener->status == RAD_LISTEN_STATUS_CLOSED) return 0;
+       if (listener->status == RAD_LISTEN_STATUS_EOL) return 0;
 
        len = write(listener->fd, buffer, len);
        if (len <= 0) command_close_socket(listener);
index fb86c92..94979c8 100644 (file)
@@ -458,7 +458,7 @@ static int dual_tcp_recv(rad_listen_t *listener)
        }
 
        if (rcode < 0) {        /* error or connection reset */
-               listener->status = RAD_LISTEN_STATUS_REMOVE_FD;
+               listener->status = RAD_LISTEN_STATUS_REMOVE_NOW;
 
                /*
                 *      Decrement the number of connections.
@@ -1848,7 +1848,7 @@ static int proxy_socket_tcp_recv(rad_listen_t *listener)
 
        packet = fr_tcp_recv(listener->fd, 0);
        if (!packet) {
-               listener->status = RAD_LISTEN_STATUS_REMOVE_FD;
+               listener->status = RAD_LISTEN_STATUS_REMOVE_NOW;
                event_new_fd(listener);
                return 0;
        }
index 65d114b..282e38f 100644 (file)
@@ -208,7 +208,7 @@ STATE_MACHINE_DECL(proxy_wait_for_reply);
 STATE_MACHINE_DECL(proxy_running);
 static int process_proxy_reply(REQUEST *request);
 static void remove_from_proxy_hash(REQUEST *request);
-static void remove_from_proxy_hash_nl(REQUEST *request);
+static void remove_from_proxy_hash_nl(REQUEST *request, bool yank);
 static int insert_into_proxy_hash(REQUEST *request);
 #endif
 
@@ -1528,7 +1528,7 @@ static void tcp_socket_timer(void *ctx)
 
                do_close:
 
-                       listener->status = RAD_LISTEN_STATUS_CLOSED;
+                       listener->status = RAD_LISTEN_STATUS_EOL;
                        event_new_fd(listener);
                        return;
                }
@@ -1593,15 +1593,17 @@ static void add_jitter(struct timeval *when)
        tv_add(when, jitter);
 }
 
-
-static int disconnect_all_proxied_requests(void *ctx, void *data)
+/*
+ *     Called by socket_del to remove requests with this socket
+ */
+static int eol_proxy_listener(void *ctx, void *data)
 {
        rad_listen_t *this = ctx;
        RADIUS_PACKET **proxy_p = data;
        REQUEST *request;
 
        request = fr_packet2myptr(REQUEST, proxy, proxy_p);
-       if (request->proxy->sockfd != this->fd) return 0;
+       if (request->proxy_listener != this) return 0;
 
        /*
         *      The normal "remove_from_proxy_hash" tries to grab the
@@ -1609,27 +1611,28 @@ static int disconnect_all_proxied_requests(void *ctx, void *data)
         *      again will cause a deadlock.  Instead, call the "no
         *      lock" version of the function.
         */
-       if (request->in_proxy_hash) {
-               remove_from_proxy_hash_nl(request);
-       }
-       request->proxy_listener = NULL;
+       rad_assert(request->in_proxy_hash == true);
+       remove_from_proxy_hash_nl(request, false);
 
        /*
         *      Don't mark it as DONE.  The client can retransmit, and
         *      the packet SHOULD be re-proxied somewhere else.
+        *
+        *      Return "2" means that the rbtree code will remove it
+        *      from the tree, and we don't need to do it ourselves.
         */
-       return 0;
+       return 2;
 }
 #endif /* WITH_PROXY */
 
-static int remove_all_requests(void *ctx, void *data)
+static int eol_listener(void *ctx, void *data)
 {
        rad_listen_t *this = ctx;
        RADIUS_PACKET **packet_p = data;
        REQUEST *request;
 
        request = fr_packet2myptr(REQUEST, packet, packet_p);
-       if (request->packet->sockfd != this->fd) return 0;
+       if (request->listener != this) return 0;
 
        request->master_state = REQUEST_STOP_PROCESSING;
 
@@ -1644,11 +1647,11 @@ static int remove_all_requests(void *ctx, void *data)
  *
  ***********************************************************************/
 
-static void remove_from_proxy_hash_nl(REQUEST *request)
+static void remove_from_proxy_hash_nl(REQUEST *request, bool yank)
 {
        if (!request->in_proxy_hash) return;
 
-       fr_packet_list_id_free(proxy_list, request->proxy);
+       fr_packet_list_id_free(proxy_list, request->proxy, yank);
        request->in_proxy_hash = false;
 
        /*
@@ -1707,7 +1710,7 @@ static void remove_from_proxy_hash(REQUEST *request)
                return;
        }
 
-       remove_from_proxy_hash_nl(request);
+       remove_from_proxy_hash_nl(request, true);
 
        PTHREAD_MUTEX_UNLOCK(&proxy_mutex);
 }
@@ -3674,144 +3677,28 @@ int event_new_fd(rad_listen_t *this)
 
                this->status = RAD_LISTEN_STATUS_KNOWN;
                return 1;
-       }
-
-       /*
-        *      Something went wrong with the socket: make it harmless.
-        */
-       if (this->status == RAD_LISTEN_STATUS_REMOVE_FD) {
-               int devnull;
-
-               /*
-                *      Remove it from the list of live FD's.
-                */
-               FD_MUTEX_LOCK(&fd_mutex);
-               fr_event_fd_delete(el, 0, this->fd);
-               FD_MUTEX_UNLOCK(&fd_mutex);
-
-#ifdef WITH_TCP
-               /*
-                *      We track requests using this socket only for
-                *      TCP.  For UDP, we don't currently close
-                *      sockets.
-                */
-#ifdef WITH_PROXY
-               if (this->type != RAD_LISTEN_PROXY)
-#endif
-               {
-                       if (this->count != 0) {
-                               fr_packet_list_walk(pl, this,
-                                                   remove_all_requests);
-                       }
-
-                       if (this->count == 0) {
-                               this->status = RAD_LISTEN_STATUS_FINISH;
-                               goto finish;
-                       }
-               }
-#ifdef WITH_PROXY
-               else {
-                       int count;
-
-                       /*
-                        *      Duplicate code
-                        */
-                       PTHREAD_MUTEX_LOCK(&proxy_mutex);
-                       if (!fr_packet_list_socket_freeze(proxy_list,
-                                                         this->fd)) {
-                               ERROR("Fatal error freezing socket: %s",
-                                      fr_strerror());
-                               exit(1);
-                       }
-
-                       /*
-                        *      Doing this with the proxy mutex held
-                        *      is a Bad Thing.  We should move to
-                        *      finer-grained mutexes.
-                        */
-                       count = this->count;
-                       if (count > 0) {
-                               fr_packet_list_walk(proxy_list, this,
-                                                   disconnect_all_proxied_requests);
-                       }
-                       count = this->count; /* protected by mutex */
-                       PTHREAD_MUTEX_UNLOCK(&proxy_mutex);
-
-                       if (count == 0) {
-                               this->status = RAD_LISTEN_STATUS_FINISH;
-                               goto finish;
-                       }
-               }
-#endif /* WITH_PROXY */
-#endif /* WITH_TCP */
-
-               /*
-                *      Re-open the socket, pointing it to /dev/null.
-                *      This means that all writes proceed without
-                *      blocking, and all reads return "no data".
-                *
-                *      This leaves the socket active, so any child
-                *      threads won't go insane.  But it means that
-                *      they cannot send or receive any packets.
-                *
-                *      This is EXTRA work in the normal case, when
-                *      sockets are closed without error.  But it lets
-                *      us have one simple processing method for all
-                *      sockets.
-                */
-               devnull = open("/dev/null", O_RDWR);
-               if (devnull < 0) {
-                       ERROR("FATAL failure opening /dev/null: %s",
-                              strerror(errno));
-                       exit(1);
-               }
-               if (dup2(devnull, this->fd) < 0) {
-                       ERROR("FATAL failure closing socket: %s",
-                              strerror(errno));
-                       exit(1);
-               }
-               close(devnull);
-
-               this->status = RAD_LISTEN_STATUS_CLOSED;
-
-               /*
-                *      Fall through to the next section.
-                */
-       }
+       } /* end of INIT */
 
 #ifdef WITH_TCP
        /*
-        *      Called ONLY from the main thread.  On the following
-        *      conditions:
-        *
-        *      idle timeout
-        *      max lifetime
-        *
-        *      (and falling through from "forcibly close FD" above)
-        *      client closed connection on us
-        *      client sent us a bad packet.
+        *      Stop using this socket, if at all possible.
         */
-       if (this->status == RAD_LISTEN_STATUS_CLOSED) {
-               int count = this->count;
-
-#ifdef WITH_DETAIL
-               rad_assert(this->type != RAD_LISTEN_DETAIL);
-#endif
-
+       if (this->status == RAD_LISTEN_STATUS_EOL) {
 #ifdef WITH_PROXY
                /*
-                *      Remove it from the list of active sockets, so
-                *      that it isn't used when proxying new packets.
+                *      Proxy sockets get frozen, so that we don't use
+                *      them for new requests.  But we do keep them
+                *      open to listen for replies to requests we had
+                *      previously sent.
                 */
                if (this->type == RAD_LISTEN_PROXY) {
                        PTHREAD_MUTEX_LOCK(&proxy_mutex);
                        if (!fr_packet_list_socket_freeze(proxy_list,
                                                          this->fd)) {
-                               ERROR("Fatal error freezing socket: %s",
+                               radlog(L_ERR, "Fatal error freezing socket: %s",
                                       fr_strerror());
                                exit(1);
                        }
-                       count = this->count; /* protected by mutex */
                        PTHREAD_MUTEX_UNLOCK(&proxy_mutex);
                }
 #endif
@@ -3820,7 +3707,7 @@ int event_new_fd(rad_listen_t *this)
                 *      Requests are still using the socket.  Wait for
                 *      them to finish.
                 */
-               if (count != 0) {
+               if (this->count > 0) {
                        struct timeval when;
                        listen_socket_t *sock = this->data;
 
@@ -3841,48 +3728,91 @@ int event_new_fd(rad_listen_t *this)
                }
 
                /*
-                *      No one is using this socket: we can delete it
-                *      immediately.
+                *      No one is using the socket.  We can remove it now.
                 */
-               this->status = RAD_LISTEN_STATUS_FINISH;
-       }
+               this->status = RAD_LISTEN_STATUS_REMOVE_NOW;
+       } /* socket is at EOL */
+#endif
 
-finish:
-       if (this->status == RAD_LISTEN_STATUS_FINISH) {
+       /*
+        *      Nuke the socket.
+        */
+       if (this->status == RAD_LISTEN_STATUS_REMOVE_NOW) {
+               int devnull;
+#ifdef WITH_TCP
                listen_socket_t *sock = this->data;
-
-               rad_assert(this->count == 0);
-               INFO(" ... closing socket %s", buffer);
+#endif
 
                /*
-                *      Remove it from the list of live FD's.  Note
-                *      that it MAY also have been removed above.  We
-                *      do it again here, to catch the case of sockets
-                *      closing on idle timeout, or max
-                *      lifetime... AFTER all requests have finished
-                *      using it.
+                *      Remove it from the list of live FD's.
                 */
                FD_MUTEX_LOCK(&fd_mutex);
                fr_event_fd_delete(el, 0, this->fd);
                FD_MUTEX_UNLOCK(&fd_mutex);
 
+               /*
+                *      Re-open the socket, pointing it to /dev/null.
+                *      This means that all writes proceed without
+                *      blocking, and all reads return "no data".
+                *
+                *      This leaves the socket active, so any child
+                *      threads won't go insane.  But it means that
+                *      they cannot send or receive any packets.
+                *
+                *      This is EXTRA work in the normal case, when
+                *      sockets are closed without error.  But it lets
+                *      us have one simple processing method for all
+                *      sockets.
+                */
+               devnull = open("/dev/null", O_RDWR);
+               if (devnull < 0) {
+                       ERROR("FATAL failure opening /dev/null: %s",
+                              strerror(errno));
+                       exit(1);
+               }
+               if (dup2(devnull, this->fd) < 0) {
+                       ERROR("FATAL failure closing socket: %s",
+                              strerror(errno));
+                       exit(1);
+               }
+               close(devnull);
+
+#ifdef WITH_DETAIL
+               rad_assert(this->type != RAD_LISTEN_DETAIL);
+#endif
+
+#ifdef WITH_TCP
+               INFO(" ... closing socket %s", buffer);
+
 #ifdef WITH_PROXY
                /*
-                *      Remove it from the list of sockets to be used
-                *      when proxying.
+                *      The socket is dead.  Force all proxied packets
+                *      to stop using it.  And then remove it from the
+                *      list of outgoing sockets.
                 */
                if (this->type == RAD_LISTEN_PROXY) {
                        PTHREAD_MUTEX_LOCK(&proxy_mutex);
-                       if (!fr_packet_list_socket_del(proxy_list,
-                                                      this->fd, NULL)) {
+                       fr_packet_list_walk(proxy_list, this,
+                                           eol_proxy_listener);
+
+                       if (!fr_packet_list_socket_del(proxy_list, this->fd)) {
                                ERROR("Fatal error removing socket: %s",
-                                      fr_strerror());
+                                     fr_strerror());
                                exit(1);
                        }
-                       if (sock->home) sock->home->limit.num_connections--;
+                       if (sock->home &&  sock->home->limit.num_connections) {
+                               sock->home->limit.num_connections--;
+                       }
                        PTHREAD_MUTEX_UNLOCK(&proxy_mutex);
-               }
+               } else
 #endif
+               {
+                       /*
+                        *      EOL all requests using this socket.
+                        */
+                       fr_packet_list_walk(proxy_list, this,
+                                           eol_listener);
+               }
 
                /*
                 *      Remove any pending cleanups.
index 2f5c75e..46e8ac0 100644 (file)
@@ -480,7 +480,7 @@ static void deallocate_id(radclient_t *radclient)
        /*
         *      One more unused RADIUS ID.
         */
-       fr_packet_list_id_free(pl, radclient->request);
+       fr_packet_list_id_free(pl, radclient->request, true);
 
        /*
         *      If we've already sent a packet, free up the old one,
index fddd609..336563e 100644 (file)
@@ -72,7 +72,7 @@ static void tls_socket_close(rad_listen_t *listener)
 {
        listen_socket_t *sock = listener->data;
 
-       listener->status = RAD_LISTEN_STATUS_REMOVE_FD;
+       listener->status = RAD_LISTEN_STATUS_REMOVE_NOW;
        listener->tls = NULL; /* parent owns this! */
 
        if (sock->parent) {