RTR server bind review

- Hint SOCK_STREAM on getaddrinfo(), not on socket().
  I guess this means it was attempting some redundant binds on failure?
- Print the "Attempting to bind socket to address '%s', port '%s'"
  message in every addrinfo, not just the first one. (Improves log
  legibility.)
- Improve the algorithm that produces the printable version of the
  address (used to dereference ai_addr->sa_data like the previous
  commit), and fall back to whatever the user provided if it fails.
- Weird inconsistency: Most errors used to cause create_server_socket()
  to try the next addrinfo candidate, but getsockname() and listen()
  induced immediate abortion. I've no idea what that was about;
  getsockname() isn't even a mandatory step.
- Attempt to bind all addrinfo candidates; they must all succeed.
  (It used to stop on the first success.)

I'm conflicted with the last point. The old behavior appears to have
been inherited from the Linux manual page example, but doesn't make any
sense from where I'm standing.

If `/etc/hosts` defines

	127.0.0.1    localhost
	::1          localhost

And server.address is "localhost", shouldn't we attempt to bind to both
addresses?

diff --git a/src/rtr/rtr.c b/src/rtr/rtr.c
index 135d40402ae6313188b3036e5267d6d683473ba7..ae44e1ac8e6a4cf659e476846c4a056e3ba292b5 100644 (file)
--- a/src/rtr/rtr.c
+++ b/src/rtr/rtr.c
@@ -110,7 +110,7 @@ init_addrinfo(char const *hostname, char const *service,
 
        memset(&hints, 0 , sizeof(hints));
        hints.ai_family = AF_UNSPEC;
-       /* hints.ai_socktype = SOCK_DGRAM; */
+       hints.ai_socktype = SOCK_STREAM;
        hints.ai_flags |= AI_PASSIVE;
 
        if (hostname != NULL)
@@ -127,7 +127,25 @@ init_addrinfo(char const *hostname, char const *service,
        return 0;
 }
 
+static char *
+get_best_printable(struct addrinfo *addr, char const *input_addr)
+{
+       char str[INET6_ADDRSTRLEN];
+
+       if (sockaddr2str((struct sockaddr_storage *) addr->ai_addr, str))
+               return pstrdup(str);
+
+       if (input_addr != NULL)
+               return pstrdup(input_addr);
+
+       /* Failure is fine; this is just a nice-to-have. */
+       return NULL;
+}
+
 /*
+ * We want to listen to all sockets in one thread,
+ * so don't block.
+ *
  * By the way: man 2 poll says
  *
  * > The operation of poll() and ppoll() is not affected by the O_NONBLOCK flag.
@@ -164,92 +182,67 @@ set_nonblock(int fd)
  * from the clients.
  */
 static int
-create_server_socket(char const *input_addr, char const *hostname,
-    char const *service)
+create_server_socket(char const *input_addr, char const *hostname, char const *port)
 {
-       struct addrinfo *addrs;
-       struct addrinfo *addr;
-       unsigned long port;
-       int reuse;
-       int fd;
+       struct addrinfo *ais, *ai;
        struct rtr_server server;
-       int error;
-
-       reuse = 1;
-
-       error = init_addrinfo(hostname, service, &addrs);
-       if (error)
-               return error;
-
-       if (addrs != NULL)
-               pr_op_info(
-                   "Attempting to bind socket to address '%s', port '%s'.",
-                   (addrs->ai_canonname != NULL) ? addrs->ai_canonname : "any",
-                   service);
-
-       for (addr = addrs; addr != NULL; addr = addr->ai_next) {
-               fd = socket(addr->ai_family, SOCK_STREAM, 0);
-               if (fd < 0) {
-                       pr_op_err("socket() failed: %s", strerror(errno));
-                       continue;
+       char const *errmsg;
+       static const int yes = 1;
+       int err;
+
+       err = init_addrinfo(hostname, port, &ais);
+       if (err)
+               return err;
+
+       for (ai = ais; ai != NULL; ai = ai->ai_next) {
+               server.fd = -1;
+               server.addr = get_best_printable(ai, input_addr);
+               pr_op_info("[%s]:%s: Setting up socket...", server.addr, port);
+
+               server.fd = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol);
+               if (server.fd < 0) {
+                       err = errno;
+                       errmsg = "Unable to create socket";
+                       goto fail;
                }
 
-               /*
-                * We want to listen to all sockets in one thread,
-                * so don't block.
-                */
-               if (set_nonblock(fd) != 0) {
-                       close(fd);
-                       continue;
+               if ((err = set_nonblock(server.fd)) != 0) {
+                       errmsg = "Unable to disable blocking on the socket";
+                       goto fail;
                }
 
-               /* enable SO_REUSEADDR */
-               if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &reuse,
-                   sizeof(int)) < 0) {
-                       pr_op_err("setsockopt(SO_REUSEADDR) failed: %s",
-                           strerror(errno));
-                       close(fd);
-                       continue;
+               if (setsockopt(server.fd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(yes)) < 0) {
+                       err = errno;
+                       errmsg = "Unable to enable SO_REUSEADDR on the socket";
+                       goto fail;
                }
 
-               if (bind(fd, addr->ai_addr, addr->ai_addrlen) < 0) {
-                       pr_op_err("bind() failed: %s", strerror(errno));
-                       close(fd);
-                       continue;
-               }
-
-               if (getsockname(fd, addr->ai_addr, &addr->ai_addrlen) != 0) {
-                       error = errno;
-                       close(fd);
-                       freeaddrinfo(addrs);
-                       pr_op_err("getsockname() failed: %s", strerror(error));
-                       return error;
+               if (bind(server.fd, ai->ai_addr, ai->ai_addrlen) < 0) {
+                       err = errno;
+                       errmsg = "Unable to bind the socket";
+                       goto fail;
                }
 
-               port = (unsigned char)(addr->ai_addr->sa_data[0]) << 8;
-               port += (unsigned char)(addr->ai_addr->sa_data[1]);
-               pr_op_info("Success; bound to address '%s', port '%ld'.",
-                   (addr->ai_canonname != NULL) ? addr->ai_canonname : "any",
-                   port);
-               freeaddrinfo(addrs);
-
-               if (listen(fd, config_get_server_queue()) != 0) {
-                       error = errno;
-                       close(fd);
-                       pr_op_err("listen() failure: %s", strerror(error));
-                       return error;
+               if (listen(server.fd, config_get_server_queue()) < 0) {
+                       err = errno;
+                       errmsg = "Unable to start listening on socket";
+                       goto fail;
                }
 
-               server.fd = fd;
-               /* Ignore failure; this is just a nice-to-have. */
-               server.addr = (input_addr != NULL) ? pstrdup(input_addr) : NULL;
+               pr_op_info("[%s]:%s: Success.", server.addr, port);
                server_arraylist_add(&servers, &server);
-
-               return 0; /* Happy path */
        }
 
-       freeaddrinfo(addrs);
-       return pr_op_err("None of the addrinfo candidates could be bound.");
+       freeaddrinfo(ais);
+       return 0;
+
+fail:
+       pr_op_err("[%s]:%s: %s: %s", server.addr, port, errmsg, strerror(err));
+       if (server.fd != -1)
+               close(server.fd);
+       free(server.addr);
+       freeaddrinfo(ais);
+       return err;
 }
 
 static int

diff --git a/src/types/address.c b/src/types/address.c
index ee1c56d9177e994cae845707576ae59f8a7cc5d7..911015fb48b0cd45e42fb38ab57eb3e35ba18000 100644 (file)
--- a/src/types/address.c
+++ b/src/types/address.c
@@ -530,7 +530,7 @@ addr2str6(struct in6_addr const *addr, char *buffer)
 /**
  * buffer must length INET6_ADDRSTRLEN.
  */
-void
+bool
 sockaddr2str(struct sockaddr_storage *sockaddr, char *buffer)
 {
        void *addr = NULL;
@@ -538,7 +538,7 @@ sockaddr2str(struct sockaddr_storage *sockaddr, char *buffer)
 
        if (sockaddr == NULL) {
                strcpy(buffer, "(null)");
-               return;
+               return false;
        }
 
        switch (sockaddr->ss_family) {
@@ -550,10 +550,14 @@ sockaddr2str(struct sockaddr_storage *sockaddr, char *buffer)
                break;
        default:
                strcpy(buffer, "(protocol unknown)");
-               return;
+               return false;
        }
 
        str = inet_ntop(sockaddr->ss_family, addr, buffer, INET6_ADDRSTRLEN);
-       if (str == NULL)
+       if (str == NULL) {
                strcpy(buffer, "(unprintable address)");
+               return false;
+       }
+
+       return true;
 }

diff --git a/src/types/address.h b/src/types/address.h
index d695e0d2aa71b22c659829341987322cfeaa6d8c..749a6565bb2e36ee99ffc759e3413530b06eb7b3 100644 (file)
--- a/src/types/address.h
+++ b/src/types/address.h
@@ -53,6 +53,6 @@ bool ipv6_covered(struct in6_addr const *, uint8_t, struct in6_addr const *);
 
 char const *addr2str4(struct in_addr const *, char *);
 char const *addr2str6(struct in6_addr const *, char *);
-void sockaddr2str(struct sockaddr_storage *, char *buffer);
+bool sockaddr2str(struct sockaddr_storage *, char *buffer);
 
 #endif /* SRC_TYPES_ADDRESS_H_ */