diff mbox series

[v5,01/10] daemon: libify socket setup and option functions

Message ID 74b0de14185120c9d53d7470e59f57fa20a1927f.1673475190.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Enhance credential helper protocol to include auth headers | expand

Commit Message

Matthew John Cheetham Jan. 11, 2023, 10:13 p.m. UTC
From: Matthew John Cheetham <mjcheetham@outlook.com>

Extract functions for setting up listening sockets and keep-alive options
from `daemon.c` to new `daemon-utils.{c,h}` files. Remove direct
dependencies on global state by inlining the behaviour at the callsites
for all libified functions.

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 Makefile       |   1 +
 daemon-utils.c | 209 +++++++++++++++++++++++++++++++++++++++++++++++
 daemon-utils.h |  23 ++++++
 daemon.c       | 214 +------------------------------------------------
 4 files changed, 237 insertions(+), 210 deletions(-)
 create mode 100644 daemon-utils.c
 create mode 100644 daemon-utils.h

Comments

Victoria Dye Jan. 12, 2023, 7:35 p.m. UTC | #1
Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
> 
> Extract functions for setting up listening sockets and keep-alive options
> from `daemon.c` to new `daemon-utils.{c,h}` files. Remove direct
> dependencies on global state by inlining the behaviour at the callsites
> for all libified functions.

Thanks for making this change, the reduced code duplication should make the
common daemon-related code more maintainable.

For reference, I used 

'git blame -s -b -C -C -C master..<this patch> -- daemon-utils.c' 

to help identify which lines in 'daemon-utils.c' were changed from their
original implementation in 'daemon.c'. I'll try to rearrange the diff to
show those differences more directly.

The first main change I see is that 'logerror' and 'reuseaddr' are changed
from global references to arguments in 'set_keep_alive()',
'setup_named_sock()' (same for 'NO_IPV6' defined and undefined), and
'socksetup()':

> -static void set_keep_alive(int sockfd)
> +void set_keep_alive(int sockfd, log_fn logerror)

> -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
> +static int setup_named_sock(char *listen_addr, int listen_port,
> +			    struct socketlist *socklist, int reuseaddr,
> +			    log_fn logerror)

> -static void socksetup(struct string_list *listen_addr, int listen_port, struct socketlist *socklist)
> +void socksetup(struct string_list *listen_addr, int listen_port,
> +	       struct socketlist *socklist, int reuseaddr,
> +	       log_fn logerror)

The external calls in 'daemon.c' to 'set_keep_alive()' and 'socksetup()' are
updated to pass the  'logerror()' function and global 'reuseaddr' as
arguments, so there isn't any change in behavior.

> @@ -759,7 +748,7 @@ static int execute(void)
>  	if (addr)
>  		loginfo("Connection from %s:%s", addr, port);
>  
> -	set_keep_alive(0);
> +	set_keep_alive(0, logerror);
>  	alarm(init_timeout ? init_timeout : timeout);
>  	pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
>  	alarm(0);
> @@ -1246,7 +1039,8 @@ static int serve(struct string_list *listen_addr, int listen_port,
>  {
>  	struct socketlist socklist = { NULL, 0, 0 };
>  
> -	socksetup(listen_addr, listen_port, &socklist);
> +	socksetup(listen_addr, listen_port, &socklist, reuseaddr,
> +		  logerror);
>  	if (socklist.nr == 0)
>  		die("unable to allocate any listen sockets on port %u",
>  		    listen_port);

The other notable change is moving the 'if (!reusaddr) return 0' block in
'set_reuse_addr()' to its callers in both 'setup_named_sock()'s:

> +#ifndef NO_IPV6
> +
> +static int setup_named_sock(char *listen_addr, int listen_port,
> +			    struct socketlist *socklist, int reuseaddr,
> +			    log_fn logerror)
> +{
...
> +		if (reuseaddr && set_reuse_addr(sockfd)) {
> +			logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> +			close(sockfd);
> +			continue;
> +		}
...
> +}
> +
> +#else /* NO_IPV6 */
> +
> +static int setup_named_sock(char *listen_addr, int listen_port,
> +			    struct socketlist *socklist, int reuseaddr,
> +			    log_fn logerror)
> +{
...
> +	if (reuseaddr && set_reuse_addr(sockfd)) {
> +		logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> +		close(sockfd);
> +		return 0;
> +	}
...
> +}
> +
> +#endif

Where, previously, that region looked like:

> -#ifndef NO_IPV6
> -
> -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
> -{
...
> -		if (set_reuse_addr(sockfd)) {
> -			logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> -			close(sockfd);
> -			continue;
> -		}
...
> -}
> -
> -#else /* NO_IPV6 */
> -
> -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
> -{
...
> -	if (set_reuse_addr(sockfd)) {
> -		logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
> -		close(sockfd);
> -		return 0;
> -	}
...
> -}
> -
> -#endif

'reuseaddr' is passed into 'setup_named_sock()' from 'socksetup()' calls in
'daemon.c', so this also won't result in changed behavior.

Otherwise, you only expose functions & types that are called in 'daemon.c'
(the rest are still static), and everything else is a verbatim copy. Looks
good to me!
Derrick Stolee Jan. 12, 2023, 8:22 p.m. UTC | #2
On 1/12/2023 2:35 PM, Victoria Dye wrote:
> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Extract functions for setting up listening sockets and keep-alive options
>> from `daemon.c` to new `daemon-utils.{c,h}` files. Remove direct
>> dependencies on global state by inlining the behaviour at the callsites
>> for all libified functions.
> 
> Thanks for making this change, the reduced code duplication should make the
> common daemon-related code more maintainable.
> 
> For reference, I used 
> 
> 'git blame -s -b -C -C -C master..<this patch> -- daemon-utils.c' 
> 
> to help identify which lines in 'daemon-utils.c' were changed from their
> original implementation in 'daemon.c'.

Neat trick! Thanks for sharing. Using --color-moved was giving similar
results, but with a lot more tracking back-and-forth to see what the
differences were.

I agree with your assessment on this patch that the differences are
valid, safe, and desired.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index b258fdbed86..2654094dbb5 100644
--- a/Makefile
+++ b/Makefile
@@ -1003,6 +1003,7 @@  LIB_OBJS += credential.o
 LIB_OBJS += csum-file.o
 LIB_OBJS += ctype.o
 LIB_OBJS += date.o
+LIB_OBJS += daemon-utils.o
 LIB_OBJS += decorate.o
 LIB_OBJS += delta-islands.o
 LIB_OBJS += diagnose.o
diff --git a/daemon-utils.c b/daemon-utils.c
new file mode 100644
index 00000000000..b96b55962db
--- /dev/null
+++ b/daemon-utils.c
@@ -0,0 +1,209 @@ 
+#include "cache.h"
+#include "daemon-utils.h"
+
+void set_keep_alive(int sockfd, log_fn logerror)
+{
+	int ka = 1;
+
+	if (setsockopt(sockfd, SOL_SOCKET, SO_KEEPALIVE, &ka, sizeof(ka)) < 0) {
+		if (errno != ENOTSOCK)
+			logerror("unable to set SO_KEEPALIVE on socket: %s",
+				strerror(errno));
+	}
+}
+
+static int set_reuse_addr(int sockfd)
+{
+	int on = 1;
+
+	return setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR,
+			  &on, sizeof(on));
+}
+
+static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
+{
+#ifdef NO_IPV6
+	static char ip[INET_ADDRSTRLEN];
+#else
+	static char ip[INET6_ADDRSTRLEN];
+#endif
+
+	switch (family) {
+#ifndef NO_IPV6
+	case AF_INET6:
+		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
+		break;
+#endif
+	case AF_INET:
+		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
+		break;
+	default:
+		xsnprintf(ip, sizeof(ip), "<unknown>");
+	}
+	return ip;
+}
+
+#ifndef NO_IPV6
+
+static int setup_named_sock(char *listen_addr, int listen_port,
+			    struct socketlist *socklist, int reuseaddr,
+			    log_fn logerror)
+{
+	int socknum = 0;
+	char pbuf[NI_MAXSERV];
+	struct addrinfo hints, *ai0, *ai;
+	int gai;
+	long flags;
+
+	xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port);
+	memset(&hints, 0, sizeof(hints));
+	hints.ai_family = AF_UNSPEC;
+	hints.ai_socktype = SOCK_STREAM;
+	hints.ai_protocol = IPPROTO_TCP;
+	hints.ai_flags = AI_PASSIVE;
+
+	gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
+	if (gai) {
+		logerror("getaddrinfo() for %s failed: %s", listen_addr, gai_strerror(gai));
+		return 0;
+	}
+
+	for (ai = ai0; ai; ai = ai->ai_next) {
+		int sockfd;
+
+		sockfd = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol);
+		if (sockfd < 0)
+			continue;
+		if (sockfd >= FD_SETSIZE) {
+			logerror("Socket descriptor too large");
+			close(sockfd);
+			continue;
+		}
+
+#ifdef IPV6_V6ONLY
+		if (ai->ai_family == AF_INET6) {
+			int on = 1;
+			setsockopt(sockfd, IPPROTO_IPV6, IPV6_V6ONLY,
+				   &on, sizeof(on));
+			/* Note: error is not fatal */
+		}
+#endif
+
+		if (reuseaddr && set_reuse_addr(sockfd)) {
+			logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
+			close(sockfd);
+			continue;
+		}
+
+		set_keep_alive(sockfd, logerror);
+
+		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
+			logerror("Could not bind to %s: %s",
+				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 strerror(errno));
+			close(sockfd);
+			continue;	/* not fatal */
+		}
+		if (listen(sockfd, 5) < 0) {
+			logerror("Could not listen to %s: %s",
+				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 strerror(errno));
+			close(sockfd);
+			continue;	/* not fatal */
+		}
+
+		flags = fcntl(sockfd, F_GETFD, 0);
+		if (flags >= 0)
+			fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
+
+		ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
+		socklist->list[socklist->nr++] = sockfd;
+		socknum++;
+	}
+
+	freeaddrinfo(ai0);
+
+	return socknum;
+}
+
+#else /* NO_IPV6 */
+
+static int setup_named_sock(char *listen_addr, int listen_port,
+			    struct socketlist *socklist, int reuseaddr,
+			    log_fn logerror)
+{
+	struct sockaddr_in sin;
+	int sockfd;
+	long flags;
+
+	memset(&sin, 0, sizeof sin);
+	sin.sin_family = AF_INET;
+	sin.sin_port = htons(listen_port);
+
+	if (listen_addr) {
+		/* Well, host better be an IP address here. */
+		if (inet_pton(AF_INET, listen_addr, &sin.sin_addr.s_addr) <= 0)
+			return 0;
+	} else {
+		sin.sin_addr.s_addr = htonl(INADDR_ANY);
+	}
+
+	sockfd = socket(AF_INET, SOCK_STREAM, 0);
+	if (sockfd < 0)
+		return 0;
+
+	if (reuseaddr && set_reuse_addr(sockfd)) {
+		logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
+		close(sockfd);
+		return 0;
+	}
+
+	set_keep_alive(sockfd, logerror);
+
+	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
+		logerror("Could not bind to %s: %s",
+			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 strerror(errno));
+		close(sockfd);
+		return 0;
+	}
+
+	if (listen(sockfd, 5) < 0) {
+		logerror("Could not listen to %s: %s",
+			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 strerror(errno));
+		close(sockfd);
+		return 0;
+	}
+
+	flags = fcntl(sockfd, F_GETFD, 0);
+	if (flags >= 0)
+		fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
+
+	ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
+	socklist->list[socklist->nr++] = sockfd;
+	return 1;
+}
+
+#endif
+
+void socksetup(struct string_list *listen_addr, int listen_port,
+	       struct socketlist *socklist, int reuseaddr,
+	       log_fn logerror)
+{
+	if (!listen_addr->nr)
+		setup_named_sock(NULL, listen_port, socklist, reuseaddr,
+				 logerror);
+	else {
+		int i, socknum;
+		for (i = 0; i < listen_addr->nr; i++) {
+			socknum = setup_named_sock(listen_addr->items[i].string,
+						   listen_port, socklist, reuseaddr,
+						   logerror);
+
+			if (socknum == 0)
+				logerror("unable to allocate any listen sockets for host %s on port %u",
+					 listen_addr->items[i].string, listen_port);
+		}
+	}
+}
diff --git a/daemon-utils.h b/daemon-utils.h
new file mode 100644
index 00000000000..6710a2a6dc0
--- /dev/null
+++ b/daemon-utils.h
@@ -0,0 +1,23 @@ 
+#ifndef DAEMON_UTILS_H
+#define DAEMON_UTILS_H
+
+#include "git-compat-util.h"
+#include "string-list.h"
+
+typedef void (*log_fn)(const char *msg, ...);
+
+struct socketlist {
+	int *list;
+	size_t nr;
+	size_t alloc;
+};
+
+/* Enable sending of keep-alive messages on the socket. */
+void set_keep_alive(int sockfd, log_fn logerror);
+
+/* Setup a number of sockets to listen on the provided addresses. */
+void socksetup(struct string_list *listen_addr, int listen_port,
+	       struct socketlist *socklist, int reuseaddr,
+	       log_fn logerror);
+
+#endif
diff --git a/daemon.c b/daemon.c
index 0ae7d12b5c1..1ed4e705680 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,9 +1,9 @@ 
 #include "cache.h"
 #include "config.h"
+#include "daemon-utils.h"
 #include "pkt-line.h"
 #include "run-command.h"
 #include "strbuf.h"
-#include "string-list.h"
 
 #ifdef NO_INITGROUPS
 #define initgroups(x, y) (0) /* nothing */
@@ -737,17 +737,6 @@  static void hostinfo_clear(struct hostinfo *hi)
 	strbuf_release(&hi->tcp_port);
 }
 
-static void set_keep_alive(int sockfd)
-{
-	int ka = 1;
-
-	if (setsockopt(sockfd, SOL_SOCKET, SO_KEEPALIVE, &ka, sizeof(ka)) < 0) {
-		if (errno != ENOTSOCK)
-			logerror("unable to set SO_KEEPALIVE on socket: %s",
-				strerror(errno));
-	}
-}
-
 static int execute(void)
 {
 	char *line = packet_buffer;
@@ -759,7 +748,7 @@  static int execute(void)
 	if (addr)
 		loginfo("Connection from %s:%s", addr, port);
 
-	set_keep_alive(0);
+	set_keep_alive(0, logerror);
 	alarm(init_timeout ? init_timeout : timeout);
 	pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
 	alarm(0);
@@ -938,202 +927,6 @@  static void child_handler(int signo)
 	signal(SIGCHLD, child_handler);
 }
 
-static int set_reuse_addr(int sockfd)
-{
-	int on = 1;
-
-	if (!reuseaddr)
-		return 0;
-	return setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR,
-			  &on, sizeof(on));
-}
-
-struct socketlist {
-	int *list;
-	size_t nr;
-	size_t alloc;
-};
-
-static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
-{
-#ifdef NO_IPV6
-	static char ip[INET_ADDRSTRLEN];
-#else
-	static char ip[INET6_ADDRSTRLEN];
-#endif
-
-	switch (family) {
-#ifndef NO_IPV6
-	case AF_INET6:
-		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
-		break;
-#endif
-	case AF_INET:
-		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
-		break;
-	default:
-		xsnprintf(ip, sizeof(ip), "<unknown>");
-	}
-	return ip;
-}
-
-#ifndef NO_IPV6
-
-static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
-{
-	int socknum = 0;
-	char pbuf[NI_MAXSERV];
-	struct addrinfo hints, *ai0, *ai;
-	int gai;
-	long flags;
-
-	xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port);
-	memset(&hints, 0, sizeof(hints));
-	hints.ai_family = AF_UNSPEC;
-	hints.ai_socktype = SOCK_STREAM;
-	hints.ai_protocol = IPPROTO_TCP;
-	hints.ai_flags = AI_PASSIVE;
-
-	gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
-	if (gai) {
-		logerror("getaddrinfo() for %s failed: %s", listen_addr, gai_strerror(gai));
-		return 0;
-	}
-
-	for (ai = ai0; ai; ai = ai->ai_next) {
-		int sockfd;
-
-		sockfd = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol);
-		if (sockfd < 0)
-			continue;
-		if (sockfd >= FD_SETSIZE) {
-			logerror("Socket descriptor too large");
-			close(sockfd);
-			continue;
-		}
-
-#ifdef IPV6_V6ONLY
-		if (ai->ai_family == AF_INET6) {
-			int on = 1;
-			setsockopt(sockfd, IPPROTO_IPV6, IPV6_V6ONLY,
-				   &on, sizeof(on));
-			/* Note: error is not fatal */
-		}
-#endif
-
-		if (set_reuse_addr(sockfd)) {
-			logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
-			close(sockfd);
-			continue;
-		}
-
-		set_keep_alive(sockfd);
-
-		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
-			logerror("Could not bind to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
-				 strerror(errno));
-			close(sockfd);
-			continue;	/* not fatal */
-		}
-		if (listen(sockfd, 5) < 0) {
-			logerror("Could not listen to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
-				 strerror(errno));
-			close(sockfd);
-			continue;	/* not fatal */
-		}
-
-		flags = fcntl(sockfd, F_GETFD, 0);
-		if (flags >= 0)
-			fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
-
-		ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
-		socklist->list[socklist->nr++] = sockfd;
-		socknum++;
-	}
-
-	freeaddrinfo(ai0);
-
-	return socknum;
-}
-
-#else /* NO_IPV6 */
-
-static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
-{
-	struct sockaddr_in sin;
-	int sockfd;
-	long flags;
-
-	memset(&sin, 0, sizeof sin);
-	sin.sin_family = AF_INET;
-	sin.sin_port = htons(listen_port);
-
-	if (listen_addr) {
-		/* Well, host better be an IP address here. */
-		if (inet_pton(AF_INET, listen_addr, &sin.sin_addr.s_addr) <= 0)
-			return 0;
-	} else {
-		sin.sin_addr.s_addr = htonl(INADDR_ANY);
-	}
-
-	sockfd = socket(AF_INET, SOCK_STREAM, 0);
-	if (sockfd < 0)
-		return 0;
-
-	if (set_reuse_addr(sockfd)) {
-		logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
-		close(sockfd);
-		return 0;
-	}
-
-	set_keep_alive(sockfd);
-
-	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
-		logerror("Could not bind to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
-			 strerror(errno));
-		close(sockfd);
-		return 0;
-	}
-
-	if (listen(sockfd, 5) < 0) {
-		logerror("Could not listen to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
-			 strerror(errno));
-		close(sockfd);
-		return 0;
-	}
-
-	flags = fcntl(sockfd, F_GETFD, 0);
-	if (flags >= 0)
-		fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
-
-	ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
-	socklist->list[socklist->nr++] = sockfd;
-	return 1;
-}
-
-#endif
-
-static void socksetup(struct string_list *listen_addr, int listen_port, struct socketlist *socklist)
-{
-	if (!listen_addr->nr)
-		setup_named_sock(NULL, listen_port, socklist);
-	else {
-		int i, socknum;
-		for (i = 0; i < listen_addr->nr; i++) {
-			socknum = setup_named_sock(listen_addr->items[i].string,
-						   listen_port, socklist);
-
-			if (socknum == 0)
-				logerror("unable to allocate any listen sockets for host %s on port %u",
-					 listen_addr->items[i].string, listen_port);
-		}
-	}
-}
-
 static int service_loop(struct socketlist *socklist)
 {
 	struct pollfd *pfd;
@@ -1246,7 +1039,8 @@  static int serve(struct string_list *listen_addr, int listen_port,
 {
 	struct socketlist socklist = { NULL, 0, 0 };
 
-	socksetup(listen_addr, listen_port, &socklist);
+	socksetup(listen_addr, listen_port, &socklist, reuseaddr,
+		  logerror);
 	if (socklist.nr == 0)
 		die("unable to allocate any listen sockets on port %u",
 		    listen_port);