On 2023/11/15 14:48:34 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Nov 15, 2023 at 02:31:19PM +0100, Omar Polo wrote:
> > These were added due to gotwebd weird struct sockaddr_storage handling.
> > Instead, we can save the size reported by getaddrinfo() and not reach
> > into the struct sockaddr storage at all (except for extracting the port
> > number for diagnostics purposes.)
> > 
> > sockets_conf_new_socket_fcgi() gets an hardcoded ipproto to 0 at the
> > moment, and for the moment keeps the hardcoded SOCK_STREAM.  cleaning up
> > this function will be the next step.
> 
> Seems a->ipproto was always 0 anyway? I don't see it being set anywhere.

AFAIK the only valid option for it in practice is PF_UNSPEC (aka 0), so
yeah, nothing new.  However, I'd still like to clean up
sockets_conf_new_socket_fcgi() a bit, saving a few flags from the
getaddrinfo() and 

> OK by me. This is a nice simplification.

thanks!  here's the follow-up.

The main idea behind saving ai_{family,socktype,protocol} from the
getaddrinfo() call is also to hopefully merge in the future the two
sockets_conf_new_* routines.  I haven't tried that yet, be we should be
able to keep only one struct sockaddr_storage for the AF_UNIX as well as
INET/INT6 usage.

this is the only diff in this batch that isn't a net-negative :/

diffstat /home/op/w/gotwebd
 M  gotwebd/gotwebd.h  |  3+   0-
 M  gotwebd/parse.y    |  7+   2-
 M  gotwebd/sockets.c  |  8+  13-

3 files changed, 18 insertions(+), 15 deletions(-)

diff /home/op/w/gotwebd
commit - cdfd248aa718819d40d0bf972b7efbb2eabd31c9
path + /home/op/w/gotwebd
blob - d5fda8da9e59b01cc49feab0b96c7ebec5256b68
file + gotwebd/gotwebd.h
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -264,6 +264,9 @@ struct address {
 	TAILQ_ENTRY(address)	 entry;
 	struct sockaddr_storage	 ss;
 	socklen_t		 slen;
+	int			 ai_family;
+	int			 ai_socktype;
+	int			 ai_protocol;
 	in_port_t		 port;
 	char			 ifname[IFNAMSIZ];
 };
blob - 4b2db2c1ab45af5408ec20a9ce4e350fa0877b05
file + gotwebd/parse.y
--- gotwebd/parse.y
+++ gotwebd/parse.y
@@ -1033,8 +1033,10 @@ get_addrs(const char *hostname, const char *servname, 
 				return (-1);
 			}
 		}
-		h->ss.ss_family = res->ai_family;
 
+		h->ai_family = res->ai_family;
+		h->ai_socktype = res->ai_socktype;
+		h->ai_protocol = res->ai_protocol;
 		memcpy(&h->ss, res->ai_addr, res->ai_addrlen);
 		h->slen = res->ai_addrlen;
 
@@ -1068,7 +1070,10 @@ addr_dup_check(struct addresslist *al, struct address 
 	const char *addrstr;
 
 	TAILQ_FOREACH(a, al, entry) {
-		if (a->slen != h->slen ||
+		if (a->ai_family != h->ai_family ||
+		    a->ai_socktype != h->ai_socktype ||
+		    a->ai_protocol != h->ai_protocol ||
+		    a->slen != h->slen ||
 		    memcmp(&a->ss, &h->ss, a->slen) != 0)
 			continue;
 
blob - dedacef4dfcf53fb027cced30494bbc5781d81c3
file + gotwebd/sockets.c
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -77,7 +77,7 @@ static void	 sockets_rlimit(int);
 static int	 sockets_dispatch_gotwebd(int, struct privsep_proc *,
 		    struct imsg *);
 static int	 sockets_unix_socket_listen(struct privsep *, struct socket *);
-static int	 sockets_create_socket(struct address *, in_port_t);
+static int	 sockets_create_socket(struct address *);
 static int	 sockets_accept_reserve(int, struct sockaddr *, socklen_t *,
 		    int, volatile int *);
 
@@ -231,6 +231,9 @@ sockets_conf_new_socket_fcgi(struct gotwebd *env, stru
 
 	memcpy(&acp->ss, &a->ss, sizeof(acp->ss));
 	acp->slen = a->slen;
+	acp->ai_family = a->ai_family;
+	acp->ai_socktype = a->ai_socktype;
+	acp->ai_protocol = a->ai_protocol;
 	acp->port = a->port;
 	if (*a->ifname != '\0') {
 		if (strlcpy(acp->ifname, a->ifname,
@@ -408,8 +411,7 @@ sockets_privinit(struct gotwebd *env, struct socket *s
 		log_debug("%s: initializing %s FCGI socket on port %d for %s",
 		    __func__, sock->conf.af_type == AF_INET ? "inet" : "inet6",
 		    sock->conf.fcgi_socket_port, sock->conf.name);
-		sock->fd = sockets_create_socket(&sock->conf.addr,
-		    sock->conf.fcgi_socket_port);
+		sock->fd = sockets_create_socket(&sock->conf.addr);
 		if (sock->fd == -1) {
 			log_warnx("%s: create FCGI socket failed", __func__);
 			return -1;
@@ -495,17 +497,11 @@ sockets_unix_socket_listen(struct privsep *ps, struct 
 }
 
 static int
-sockets_create_socket(struct address *a, in_port_t port)
+sockets_create_socket(struct address *a)
 {
-	struct addrinfo hints;
 	int fd = -1, o_val = 1, flags;
 
-	memset(&hints, 0, sizeof(hints));
-	hints.ai_family = AF_UNSPEC;
-	hints.ai_socktype = SOCK_STREAM;
-	hints.ai_flags |= AI_PASSIVE;
-
-	fd = socket(a->ss.ss_family, hints.ai_socktype, 0);
+	fd = socket(a->ai_family, a->ai_socktype, a->ai_protocol);
 	if (fd == -1)
 		return -1;
 
@@ -530,8 +526,7 @@ sockets_create_socket(struct address *a, in_port_t por
 
 	if (bind(fd, (struct sockaddr *)&a->ss, a->slen) == -1) {
 		close(fd);
-		log_info("%s: can't bind to port %d", __func__,
-		    ntohs(port));
+		log_info("%s: can't bind to port %d", __func__, a->port);
 		return -1;
 	}