threads / discuss / 24836

added possibility to supply more than one --listen argument to git-daemon

Subject: added possibility to supply more than one --listen argument to git-daemon

## tl;dr

18 messages between Aug 23, 2010 and Aug 30, 2010.

replies: 17people: 3as markdown or json

Alexander Sulfrian· Aug 23, 2010, 16:54 UTC · lore
Hi list,

I have written a patch to allow more than one --listen argument to the git daemon. The current situation is, that the daemon silently ignores all --listen arguments beside the last one. So there is no possibility to let on server listen on many addresses without let it bind to all.

Even if that is currently not a problem with ipv4 addresses, in the future that problem gets even worse. I wanted git-daemon to listen on an ipv4 address and an ipv6 address. That is currently not possible without running two instances. So I created that patch.

I already send this patch as [PATCH/RFC] to the list earlier. But because nobody has send any reply yet, I am resending it as [PATCH] ready for inclusion now.

Thanks, Alex

Alexander Sulfrian· Aug 23, 2010, 16:54 UTC · re: Alexander Sulfrian · lore

[PATCH] added possibility to supply more than one --listen argument to git-daemon

--listen arguments are gathered in a string_list serve and socksetup get listen_addr as string_list socketsetup creates a listen socket for each host in that string_list

Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
---
 daemon.c |  183 ++++++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 107 insertions(+), 76 deletions(-)
diff --git a/daemon.c b/daemon.c
index e22a2b7..f4492fe 100644
--- a/daemon.c
+++ b/daemon.c
@@ -3,6 +3,7 @@
 #include "exec_cmd.h"
 #include "run-command.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 #include <syslog.h>
 
@@ -736,7 +737,7 @@ static int set_reuse_addr(int sockfd)
 
 #ifndef NO_IPV6
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int socksetup(struct string_list *listen_addr, int listen_port, int **socklist_p)
 {
 	int socknum = 0, *socklist = NULL;
 	int maxfd = -1;
@@ -744,6 +745,7 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 	struct addrinfo hints, *ai0, *ai;
 	int gai;
 	long flags;
+	int i;
 
 	sprintf(pbuf, "%d", listen_port);
 	memset(&hints, 0, sizeof(hints));
@@ -752,57 +754,69 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 	hints.ai_protocol = IPPROTO_TCP;
 	hints.ai_flags = AI_PASSIVE;
 
-	gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
-	if (gai)
-		die("getaddrinfo() failed: %s", gai_strerror(gai));
+	i = 0;
+	do {
+		if (listen_addr->nr > 0) {
+			gai = getaddrinfo(listen_addr->items[i].string, pbuf,
+					  &hints, &ai0);
+		}
+		else {
+			gai = getaddrinfo(NULL, pbuf, &hints, &ai0);
+		}
 
-	for (ai = ai0; ai; ai = ai->ai_next) {
-		int sockfd;
+		if (gai)
+			die("getaddrinfo() failed: %s", gai_strerror(gai));
 
-		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;
-		}
+		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 */
-		}
+			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)) {
-			close(sockfd);
-			continue;
-		}
+			if (set_reuse_addr(sockfd)) {
+				close(sockfd);
+				continue;
+			}
 
-		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
-			close(sockfd);
-			continue;	/* not fatal */
-		}
-		if (listen(sockfd, 5) < 0) {
-			close(sockfd);
-			continue;	/* not fatal */
-		}
+			if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
+				close(sockfd);
+				continue;	/* not fatal */
+			}
+			if (listen(sockfd, 5) < 0) {
+				close(sockfd);
+				continue;	/* not fatal */
+			}
 
-		flags = fcntl(sockfd, F_GETFD, 0);
-		if (flags >= 0)
-			fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
+			flags = fcntl(sockfd, F_GETFD, 0);
+			if (flags >= 0)
+				fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
 
-		socklist = xrealloc(socklist, sizeof(int) * (socknum + 1));
-		socklist[socknum++] = sockfd;
+			socklist = xrealloc(socklist,
+					    sizeof(int) * (socknum + 1));
+			socklist[socknum++] = sockfd;
 
-		if (maxfd < sockfd)
-			maxfd = sockfd;
-	}
+			if (maxfd < sockfd)
+				maxfd = sockfd;
+		}
 
-	freeaddrinfo(ai0);
+		freeaddrinfo(ai0);
+	} while  (++i < listen_addr->nr);
 
 	*socklist_p = socklist;
 	return socknum;
@@ -810,50 +824,60 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 
 #else /* NO_IPV6 */
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int socksetup(struct string_list *listen_addr, int listen_port, int **socklist_p)
 {
+	int socknum = 0, *socklist = NULL;
 	struct sockaddr_in sin;
 	int sockfd;
 	long flags;
+	int i;
 
 	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)
+	i = 0;
+	do {
+		if (listen_addr->nr > 0) {
+			/* Well, host better be an IP address here. */
+			if (inet_pton(AF_INET, listen_addr->items[i].string,
+				      &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;
-	} 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)) {
+			close(sockfd);
+			return 0;
+		}
 
-	if (set_reuse_addr(sockfd)) {
-		close(sockfd);
-		return 0;
-	}
+		if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
+			close(sockfd);
+			return 0;
+		}
 
-	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
-		close(sockfd);
-		return 0;
-	}
+		if (listen(sockfd, 5) < 0) {
+			close(sockfd);
+			return 0;
+		}
 
-	if (listen(sockfd, 5) < 0) {
-		close(sockfd);
-		return 0;
-	}
+		flags = fcntl(sockfd, F_GETFD, 0);
+		if (flags >= 0)
+			fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
 
-	flags = fcntl(sockfd, F_GETFD, 0);
-	if (flags >= 0)
-		fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
+		socklist = xrealloc(socklist, sizeof(int) * (socknum + 1));
+		socklist[socknum++] = sockfd;
 
-	*socklist_p = xmalloc(sizeof(int));
-	**socklist_p = sockfd;
-	return 1;
+	} while (++i < listen_addr->nr);
+
+	*socklist_p = socklist;
+	return socknum;
 }
 
 #endif
@@ -946,14 +970,14 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }
 
-static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
+static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
 {
 	int socknum, *socklist;
 
 	socknum = socksetup(listen_addr, listen_port, &socklist);
 	if (socknum == 0)
-		die("unable to allocate any listen sockets on host %s port %u",
-		    listen_addr, listen_port);
+		die("unable to allocate any listen sockets on port %u",
+		    listen_port);
 
 	if (pass && gid &&
 	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
@@ -966,14 +990,17 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t
 int main(int argc, char **argv)
 {
 	int listen_port = 0;
-	char *listen_addr = NULL;
 	int inetd_mode = 0;
+	struct string_list listen_addr;
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
 	struct passwd *pass = NULL;
 	struct group *group;
 	gid_t gid = 0;
 	int i;
+	int return_value;
+
+	memset(&listen_addr, 0, sizeof(struct string_list));
 
 	git_extract_argv0_path(argv[0]);
 
@@ -981,7 +1008,7 @@ int main(int argc, char **argv)
 		char *arg = argv[i];
 
 		if (!prefixcmp(arg, "--listen=")) {
-			listen_addr = xstrdup_tolower(arg + 9);
+			string_list_append(&listen_addr, xstrdup_tolower(arg + 9));
 			continue;
 		}
 		if (!prefixcmp(arg, "--port=")) {
@@ -1106,7 +1133,7 @@ int main(int argc, char **argv)
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
 
-	if (inetd_mode && (listen_port || listen_addr))
+	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;
@@ -1161,5 +1188,9 @@ int main(int argc, char **argv)
 	if (pid_file)
 		store_pid(pid_file);
 
-	return serve(listen_addr, listen_port, pass, gid);
+	return_value = serve(&listen_addr, listen_port, pass, gid);
+
+	string_list_clear(&listen_addr, 0);
+
+	return return_value;
 }
-- 
1.7.1
Junio C Hamano· Aug 23, 2010, 19:56 UTC · re: Alexander Sulfrian · lore

Re: [PATCH] added possibility to supply more than one --listen argument to git-daemon

Alexander Sulfrian <alexander@sulfrian.net> writes:
Show 6 quoted lines
> --listen arguments are gathered in a string_list
> serve and socksetup get listen_addr as string_list
> socketsetup creates a listen socket for each host in that string_list
>
> Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
> ---

Thanks for a resend/reminder. I've been meaning to look at this patch after somebody who actually runs the daemon commented on it.

A few administravia:
 - Please begin the subject line with affected-area and a colon;
 - Please place more stress on what problem the patch tries to solve and
   less on how the patch solves it, because the latter can be read from
   the patch text itself, when writing a proposed log message;
 - We tend to write the log message in imperative mood, to order either the
   person who applies the patch or the codebase what new things to do.
 - We need to also update the documentation (e.g. just add "Can be given
   more than one times" before "Incompatible with '--inetd' option." in
   Documentation/git-daemon.txt).
So...
    Subject: [PATCH] daemon: allow more than one host addresses given via --listen
    When the host has more than one interfaces, daemon can listen to all
    of them by not giving any --listen option, or listen to only one.
    Teach it to accept more than one --listen options.
    Signed-off-by: ...
Show 48 quoted lines
>  daemon.c |  183 ++++++++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 107 insertions(+), 76 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index e22a2b7..f4492fe 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -3,6 +3,7 @@
>  #include "exec_cmd.h"
>  #include "run-command.h"
>  #include "strbuf.h"
> +#include "string-list.h"
>  
>  #include <syslog.h>
>  
> @@ -736,7 +737,7 @@ static int set_reuse_addr(int sockfd)
>  
>  #ifndef NO_IPV6
>  
> -static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
> +static int socksetup(struct string_list *listen_addr, int listen_port, int **socklist_p)
>  {
>  	int socknum = 0, *socklist = NULL;
>  	int maxfd = -1;
> @@ -744,6 +745,7 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
>  	struct addrinfo hints, *ai0, *ai;
>  	int gai;
>  	long flags;
> +	int i;
>  
>  	sprintf(pbuf, "%d", listen_port);
>  	memset(&hints, 0, sizeof(hints));
> @@ -752,57 +754,69 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
>  	hints.ai_protocol = IPPROTO_TCP;
>  	hints.ai_flags = AI_PASSIVE;
>  
> -	gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
> -	if (gai)
> -		die("getaddrinfo() failed: %s", gai_strerror(gai));
> +	i = 0;
> +	do {
> +		if (listen_addr->nr > 0) {
> +			gai = getaddrinfo(listen_addr->items[i].string, pbuf,
> +					  &hints, &ai0);
> +		}
> +		else {
> +			gai = getaddrinfo(NULL, pbuf, &hints, &ai0);
> +		}
Style: neither of these if/else body need braces around it.

But more importantly, wouldn't it make the patch and the result easier to read if you split the part of the code that now is one iteration of the loop over listen_addr list into a separate helper function?

Then socksetup would look something like:
        static int socksetup(...)
        {
		...
		if (!listen_addr_list->nr)
		    	setup_named_sock(NULL, listen_port, socklist_p);
		else {
			int i;
			for (i = 0; i < listen_addr_list->nr; i++)
                        	setup_named_sock(listen_addr_list->items[i].string,
                                	listen_port, socklist_p);
                }
		...
    	}

If such a refactoring results in more readable code (I haven't tried doing the refactoring myself, so I don't know if it is worth it), then I would suggest making this into a two-patch series, one that creates the helper function, and then another that adds multiple-listen support on top.

Show 15 quoted lines
> @@ -946,14 +970,14 @@ static void store_pid(const char *path)
>  		die_errno("failed to write pid file '%s'", path);
>  }
>  
> -static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
> +static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>  {
>  	int socknum, *socklist;
>  
>  	socknum = socksetup(listen_addr, listen_port, &socklist);
>  	if (socknum == 0)
> -		die("unable to allocate any listen sockets on host %s port %u",
> -		    listen_addr, listen_port);
> +		die("unable to allocate any listen sockets on port %u",
> +		    listen_port);
Here we are losing "host" information; does it matter?

Earlier socksetup() died only when getaddrinfo died, as in that case we know there won't be any socket prepared. You removed that die (which is sensible---as you may have one unavailable and another available interface and dying only when there is no socket listening has been the design of the program, and you are not changing that with this patch).

Also I have to wonder what would have happened in the original code if there were no --listen given (presumably we got NULL in the argument in that case).

My gut feeling is that this change is Ok, as this die() would trigger only when no interface out of either all interface or the ones specified on the command line with --listen options, can be listened to at the port, either given by --listen_port option or the default 9418; and the user does know which "host" was asked anyway. But the change needs to be mentioned in the proposed log message.

It might be an improvement if we reported addresses that cannot be listened to (you can use listen_addr_list->items[i].util for that--- when setup_named_sock() helper finds that no new socket was added by the loop over the list resulting from getaddrinfo, it can mark the item's util field, and then instead of dying the caller of socksetup() can warn on such names. If we were to do so, however, that should be done as a separate patch on top of this change.

Show 16 quoted lines
> @@ -966,14 +990,17 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t
>  int main(int argc, char **argv)
>  {
>  	int listen_port = 0;
> -	char *listen_addr = NULL;
>  	int inetd_mode = 0;
> +	struct string_list listen_addr;
>  	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
>  	int detach = 0;
>  	struct passwd *pass = NULL;
>  	struct group *group;
>  	gid_t gid = 0;
>  	int i;
> +	int return_value;
> +
> +	memset(&listen_addr, 0, sizeof(struct string_list));
Don't we have STRING_LIST_INIT macro these days?

I also have to wonder if we eventually want to take --listen=host:port so that you can listen only to two interfaces out of three avaiable on your host, but on different ports. Again, if we were to do so, however, that should be done as a separate patch on top of this change.

Thanks.
Alexander Sulfrian· Aug 29, 2010, 15:07 UTC · re: Junio C Hamano · lore

daemon: allow more than one host addresses given via --listen

Hi, here is version 2 of my patch. I hope i considered all of your hints Junio.

The patch currently does not support the --listen=host:port format, but I will look if it could be done. The problem that I currently see: For IPv6 it should be handle [ipv6-address]:port correctly. But anyway it should be a patch on top of this patch.

Thanks, Alex

 Documentation/git-daemon.txt | Alexander Sulfrian (2):
  daemon: add helper function named_sock_setup
  daemon: allow more than one host address given via --listen
 Documentation/git-daemon.txt |    1 +
 daemon.c                     |   62 +++++++++++++++++++++++++++++------------
 2 files changed, 45 insertions(+), 18 deletions(-)
Alexander Sulfrian· Aug 29, 2010, 15:13 UTC · re: Alexander Sulfrian · lore

[PATCHv3 1/2] daemon: add helper function named_sock_setup

Add named_sock_setup as helper function for socksetup to make it easier to create more than one listen sockets. named_sock_setup could be called more than one time and add the new sockets to the supplied socklist_p.

Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
---
 daemon.c |   35 ++++++++++++++++++++++++-----------
 1 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/daemon.c b/daemon.c
index e22a2b7..deda4cf 100644
--- a/daemon.c
+++ b/daemon.c
@@ -736,9 +736,9 @@ static int set_reuse_addr(int sockfd)
 
 #ifndef NO_IPV6
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p, int socknum)
 {
-	int socknum = 0, *socklist = NULL;
+	int *socklist = *socklist_p;
 	int maxfd = -1;
 	char pbuf[NI_MAXSERV];
 	struct addrinfo hints, *ai0, *ai;
@@ -810,8 +810,9 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 
 #else /* NO_IPV6 */
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p, int socknum)
 {
+	int *socklist = *socklist_p;
 	struct sockaddr_in sin;
 	int sockfd;
 	long flags;
@@ -823,41 +824,53 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 	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;
+			return socknum;
 	} else {
 		sin.sin_addr.s_addr = htonl(INADDR_ANY);
 	}
 
 	sockfd = socket(AF_INET, SOCK_STREAM, 0);
 	if (sockfd < 0)
-		return 0;
+		return socknum;
 
 	if (set_reuse_addr(sockfd)) {
 		close(sockfd);
-		return 0;
+		return socknum;
 	}
 
 	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
 		close(sockfd);
-		return 0;
+		return socknum;
 	}
 
 	if (listen(sockfd, 5) < 0) {
 		close(sockfd);
-		return 0;
+		return socknum;
 	}
 
 	flags = fcntl(sockfd, F_GETFD, 0);
 	if (flags >= 0)
 		fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
 
-	*socklist_p = xmalloc(sizeof(int));
-	**socklist_p = sockfd;
-	return 1;
+	socklist = xrealloc(socklist, sizeof(int) * (socknum + 1));
+	socklist[socknum++] = sockfd;
+
+	*socklist_p = socklist;
+	return socknum;
 }
 
 #endif
 
+static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+{
+	int socknum = 0, *socklist = NULL;
+
+	socknum = setup_named_sock(listen_addr, listen_port, &socklist, socknum);
+
+	*socklist_p = socklist;
+	return socknum;
+}
+
 static int service_loop(int socknum, int *socklist)
 {
 	struct pollfd *pfd;
-- 
1.7.1
Alexander Sulfrian· Aug 29, 2010, 15:13 UTC · re: Alexander Sulfrian · lore

[PATCHv3 2/2] daemon: allow more than one host address given via --listen

When the host has more than one interfaces, daemon can listen to all of them by not giving any --listen option, or listen to only one. Teach it to accept more than one --listen options.

Remove the hostname information form the die, if no socket could be created. It would only trigger when no interface out of either all interface or the ones specified on the command line with --listen options, can be listened to and so the user does know which "host" was asked.

Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
---
 Documentation/git-daemon.txt |    1 +
 daemon.c                     |   33 ++++++++++++++++++++++++---------
 2 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 01c9f8e..4afd0a4 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -85,6 +85,7 @@ OPTIONS
 	be either an IPv4 address or an IPv6 address if supported.  If IPv6
 	is not supported, then --listen=hostname is also not supported and
 	--listen must be given an IPv4 address.
+	Can be given more than one time.
 	Incompatible with '--inetd' option.
 
 --port=n::
diff --git a/daemon.c b/daemon.c
index deda4cf..bd7574e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -3,6 +3,7 @@
 #include "exec_cmd.h"
 #include "run-command.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 #include <syslog.h>
 
@@ -861,11 +862,20 @@ static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p
 
 #endif
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int socksetup(struct string_list *listen_addr, int listen_port, int **socklist_p)
 {
 	int socknum = 0, *socklist = NULL;
 
-	socknum = setup_named_sock(listen_addr, listen_port, &socklist, socknum);
+	if (!listen_addr->nr)
+		socknum = setup_named_sock(NULL, listen_port, &socklist,
+					   socknum);
+	else {
+		int i;
+		for (i = 0; i < listen_addr->nr; i++)
+			socknum = setup_named_sock(listen_addr->items[i].string,
+						   listen_port, &socklist,
+						   socknum);
+	}
 
 	*socklist_p = socklist;
 	return socknum;
@@ -959,14 +969,14 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }
 
-static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
+static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
 {
 	int socknum, *socklist;
 
 	socknum = socksetup(listen_addr, listen_port, &socklist);
 	if (socknum == 0)
-		die("unable to allocate any listen sockets on host %s port %u",
-		    listen_addr, listen_port);
+		die("unable to allocate any listen sockets on port %u",
+		    listen_port);
 
 	if (pass && gid &&
 	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
@@ -979,7 +989,7 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t
 int main(int argc, char **argv)
 {
 	int listen_port = 0;
-	char *listen_addr = NULL;
+	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
 	int inetd_mode = 0;
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
@@ -987,6 +997,7 @@ int main(int argc, char **argv)
 	struct group *group;
 	gid_t gid = 0;
 	int i;
+	int return_value;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -994,7 +1005,7 @@ int main(int argc, char **argv)
 		char *arg = argv[i];
 
 		if (!prefixcmp(arg, "--listen=")) {
-			listen_addr = xstrdup_tolower(arg + 9);
+			string_list_append(&listen_addr, xstrdup_tolower(arg + 9));
 			continue;
 		}
 		if (!prefixcmp(arg, "--port=")) {
@@ -1119,7 +1130,7 @@ int main(int argc, char **argv)
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
 
-	if (inetd_mode && (listen_port || listen_addr))
+	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;
@@ -1174,5 +1185,9 @@ int main(int argc, char **argv)
 	if (pid_file)
 		store_pid(pid_file);
 
-	return serve(listen_addr, listen_port, pass, gid);
+	return_value = serve(&listen_addr, listen_port, pass, gid);
+
+	string_list_clear(&listen_addr, 0);
+
+	return return_value;
 }
-- 
1.7.1
Junio C Hamano· Aug 30, 2010, 07:28 UTC · re: Alexander Sulfrian · lore

Re: [PATCHv3 2/2] daemon: allow more than one host address given via --listen

Alexander Sulfrian <alexander@sulfrian.net> writes:
Show 23 quoted lines
> @@ -861,11 +862,20 @@ static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p
>  
>  #endif
>  
> -static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
> +static int socksetup(struct string_list *listen_addr, int listen_port, int **socklist_p)
>  {
>  	int socknum = 0, *socklist = NULL;
>  
> -	socknum = setup_named_sock(listen_addr, listen_port, &socklist, socknum);
> +	if (!listen_addr->nr)
> +		socknum = setup_named_sock(NULL, listen_port, &socklist,
> +					   socknum);
> +	else {
> +		int i;
> +		for (i = 0; i < listen_addr->nr; i++)
> +			socknum = setup_named_sock(listen_addr->items[i].string,
> +						   listen_port, &socklist,
> +						   socknum);
> +	}
>  
>  	*socklist_p = socklist;
>  	return socknum;

Giving an old number and returning a new number feels a bit awkward as an API. If you create a structure that consists of a <pointer, nr, alloc> tuple that is suitable for ALLOC_GROW() API and pass that around instead of <&socklist, socknum> pair, then the helper can return how many sockets it prepared, and signal a failure with a negative value, no?

Show 10 quoted lines
> +static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>  {
>  	int socknum, *socklist;
>  
>  	socknum = socksetup(listen_addr, listen_port, &socklist);
>  	if (socknum == 0)
> -		die("unable to allocate any listen sockets on host %s port %u",
> -		    listen_addr, listen_port);
> +		die("unable to allocate any listen sockets on port %u",
> +		    listen_port);

The old code accepted only one --listen, so it was clear when no socket can be prepared for a given name (which may expand to multiple addresses), it is clear what failed (i.e. the failing input could have been only _one_ from the user's point of view). This check diagnoses the case where socket preparation failed for all names. Don't we want to have a new check inside the iteration over names in socksetup() to warn when socket preparation for all addresses for a name fails?

Also doesn't setup_named_sock() as refactored die() when one of the names given to --listen results no socket under ipv6 build but keeps going under noipv6 build? Without multi-listen, the distinction did not matter exactly because it took only one name, but your multi-listen addition exposes this inconsistency. I think the helper should be modified not to die but just return "I didn't prepare any socket for the given name", to allow the outer loop to continue on to the next name (perhaps while issuing a warning e.g. "No listening socket resulted for '%s'".

Alexander Sulfrian· Aug 30, 2010, 11:30 UTC · re: Junio C Hamano · lore

daemon: allow more than one host address given via --listen

Hi, here is a new version of the two patches. The sockets are now in a socketlist structure that is compatible with ALLOC_GROW(). Also if no socket could be created for one host/ip, git daemon prints now an error but continues (in both ipv6 and noipv6 builds). Only if no socket could be created at all, it will die.

Also I removed the string_list_clear (it leave back memory leaks form the xstrdup'ed-strings). It would be happen just before the exit of the application and just before the operating system cleans the memory. So I hope, that it is okay to remove it. If not i will change it, to really free all lists before exit.

Thanks Alex

Alexander Sulfrian· Aug 30, 2010, 11:30 UTC · re: Junio C Hamano · lore

[PATCHv4 1/2] daemon: add helper function named_sock_setup

Add named_sock_setup as helper function for socksetup to make it easier to create more than one listen sockets. named_sock_setup could be called more than one time and add the new sockets to the supplied socklist_p.

Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
---
 daemon.c |   53 +++++++++++++++++++++++++++++++++--------------------
 1 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/daemon.c b/daemon.c
index e22a2b7..c666ced 100644
--- a/daemon.c
+++ b/daemon.c
@@ -734,11 +734,17 @@ static int set_reuse_addr(int sockfd)
 			  &on, sizeof(on));
 }
 
+struct socketlist {
+	int *list;
+	size_t nr;
+	size_t alloc;
+};
+
 #ifndef NO_IPV6
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
 {
-	int socknum = 0, *socklist = NULL;
+	int socknum = 0;
 	int maxfd = -1;
 	char pbuf[NI_MAXSERV];
 	struct addrinfo hints, *ai0, *ai;
@@ -753,8 +759,10 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 	hints.ai_flags = AI_PASSIVE;
 
 	gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
-	if (gai)
-		die("getaddrinfo() failed: %s", gai_strerror(gai));
+	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;
@@ -795,8 +803,9 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 		if (flags >= 0)
 			fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
 
-		socklist = xrealloc(socklist, sizeof(int) * (socknum + 1));
-		socklist[socknum++] = sockfd;
+		ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
+		socklist->list[socklist->nr++] = sockfd;
+		socknum++;
 
 		if (maxfd < sockfd)
 			maxfd = sockfd;
@@ -804,13 +813,12 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 
 	freeaddrinfo(ai0);
 
-	*socklist_p = socklist;
 	return socknum;
 }
 
 #else /* NO_IPV6 */
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
 {
 	struct sockaddr_in sin;
 	int sockfd;
@@ -851,22 +859,27 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 	if (flags >= 0)
 		fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
 
-	*socklist_p = xmalloc(sizeof(int));
-	**socklist_p = sockfd;
+	ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
+	socklist->list[socklist->nr++] = sockfd;
 	return 1;
 }
 
 #endif
 
-static int service_loop(int socknum, int *socklist)
+static void socksetup(char *listen_addr, int listen_port, struct socketlist *socklist)
+{
+	setup_named_sock(listen_addr, listen_port, socklist);
+}
+
+static int service_loop(struct socketlist *socklist)
 {
 	struct pollfd *pfd;
 	int i;
 
-	pfd = xcalloc(socknum, sizeof(struct pollfd));
+	pfd = xcalloc(socklist->nr, sizeof(struct pollfd));
 
-	for (i = 0; i < socknum; i++) {
-		pfd[i].fd = socklist[i];
+	for (i = 0; i < socklist->nr; i++) {
+		pfd[i].fd = socklist->list[i];
 		pfd[i].events = POLLIN;
 	}
 
@@ -877,7 +890,7 @@ static int service_loop(int socknum, int *socklist)
 
 		check_dead_children();
 
-		if (poll(pfd, socknum, -1) < 0) {
+		if (poll(pfd, socklist->nr, -1) < 0) {
 			if (errno != EINTR) {
 				logerror("Poll failed, resuming: %s",
 				      strerror(errno));
@@ -886,7 +899,7 @@ static int service_loop(int socknum, int *socklist)
 			continue;
 		}
 
-		for (i = 0; i < socknum; i++) {
+		for (i = 0; i < socklist->nr; i++) {
 			if (pfd[i].revents & POLLIN) {
 				struct sockaddr_storage ss;
 				unsigned int sslen = sizeof(ss);
@@ -948,10 +961,10 @@ static void store_pid(const char *path)
 
 static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
 {
-	int socknum, *socklist;
+	struct socketlist socklist = { NULL, 0, 0 };
 
-	socknum = socksetup(listen_addr, listen_port, &socklist);
-	if (socknum == 0)
+	socksetup(listen_addr, listen_port, &socklist);
+	if (socklist.nr == 0)
 		die("unable to allocate any listen sockets on host %s port %u",
 		    listen_addr, listen_port);
 
@@ -960,7 +973,7 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t
 	     setuid(pass->pw_uid)))
 		die("cannot drop privileges");
 
-	return service_loop(socknum, socklist);
+	return service_loop(&socklist);
 }
 
 int main(int argc, char **argv)
-- 
1.7.1
Erik Faye-Lund· Aug 30, 2010, 12:12 UTC · re: Alexander Sulfrian · lore

Re: [PATCHv4 1/2] daemon: add helper function named_sock_setup

On Mon, Aug 30, 2010 at 1:30 PM, Alexander Sulfrian <alexander@sulfrian.net> wrote:

Show 133 quoted lines
> Add named_sock_setup as helper function for socksetup to make it
> easier to create more than one listen sockets. named_sock_setup could
> be called more than one time and add the new sockets to the supplied
> socklist_p.
>
> Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
> ---
>  daemon.c |   53 +++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index e22a2b7..c666ced 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -734,11 +734,17 @@ static int set_reuse_addr(int sockfd)
>                          &on, sizeof(on));
>  }
>
> +struct socketlist {
> +       int *list;
> +       size_t nr;
> +       size_t alloc;
> +};
> +
>  #ifndef NO_IPV6
>
> -static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
> +static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
>  {
> -       int socknum = 0, *socklist = NULL;
> +       int socknum = 0;
>        int maxfd = -1;
>        char pbuf[NI_MAXSERV];
>        struct addrinfo hints, *ai0, *ai;
> @@ -753,8 +759,10 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
>        hints.ai_flags = AI_PASSIVE;
>
>        gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
> -       if (gai)
> -               die("getaddrinfo() failed: %s", gai_strerror(gai));
> +       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;
> @@ -795,8 +803,9 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
>                if (flags >= 0)
>                        fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
>
> -               socklist = xrealloc(socklist, sizeof(int) * (socknum + 1));
> -               socklist[socknum++] = sockfd;
> +               ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
> +               socklist->list[socklist->nr++] = sockfd;
> +               socknum++;
>
>                if (maxfd < sockfd)
>                        maxfd = sockfd;
> @@ -804,13 +813,12 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
>
>        freeaddrinfo(ai0);
>
> -       *socklist_p = socklist;
>        return socknum;
>  }
>
>  #else /* NO_IPV6 */
>
> -static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
> +static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
>  {
>        struct sockaddr_in sin;
>        int sockfd;
> @@ -851,22 +859,27 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
>        if (flags >= 0)
>                fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
>
> -       *socklist_p = xmalloc(sizeof(int));
> -       **socklist_p = sockfd;
> +       ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
> +       socklist->list[socklist->nr++] = sockfd;
>        return 1;
>  }
>
>  #endif
>
> -static int service_loop(int socknum, int *socklist)
> +static void socksetup(char *listen_addr, int listen_port, struct socketlist *socklist)
> +{
> +       setup_named_sock(listen_addr, listen_port, socklist);
> +}
> +
> +static int service_loop(struct socketlist *socklist)
>  {
>        struct pollfd *pfd;
>        int i;
>
> -       pfd = xcalloc(socknum, sizeof(struct pollfd));
> +       pfd = xcalloc(socklist->nr, sizeof(struct pollfd));
>
> -       for (i = 0; i < socknum; i++) {
> -               pfd[i].fd = socklist[i];
> +       for (i = 0; i < socklist->nr; i++) {
> +               pfd[i].fd = socklist->list[i];
>                pfd[i].events = POLLIN;
>        }
>
> @@ -877,7 +890,7 @@ static int service_loop(int socknum, int *socklist)
>
>                check_dead_children();
>
> -               if (poll(pfd, socknum, -1) < 0) {
> +               if (poll(pfd, socklist->nr, -1) < 0) {
>                        if (errno != EINTR) {
>                                logerror("Poll failed, resuming: %s",
>                                      strerror(errno));
> @@ -886,7 +899,7 @@ static int service_loop(int socknum, int *socklist)
>                        continue;
>                }
>
> -               for (i = 0; i < socknum; i++) {
> +               for (i = 0; i < socklist->nr; i++) {
>                        if (pfd[i].revents & POLLIN) {
>                                struct sockaddr_storage ss;
>                                unsigned int sslen = sizeof(ss);
> @@ -948,10 +961,10 @@ static void store_pid(const char *path)
>
>  static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
>  {
> -       int socknum, *socklist;
> +       struct socketlist socklist = { NULL, 0, 0 };
>

Since serve() isn't a library function, wouldn't it reduce needless code churn to just make socklist a set of global variables (or just a global struct)? That way you don't have to pass it around, changing all those function prototypes.

I'm a bit intimidated by this change since I have a rather big patch-set on top of daemon.c, and I really don't want to resolve a lot of conflicts. But I guess that's my problem :P

AlexanderS· Aug 30, 2010, 12:46 UTC · re: Erik Faye-Lund · lore

Re: [PATCHv4 1/2] daemon: add helper function named_sock_setup

On Mon, 30 Aug 2010 14:12:52 +0200 Erik Faye-Lund <kusmabite@gmail.com> wrote:

> Since serve() isn't a library function, wouldn't it reduce needless
> code churn to just make socklist a set of global variables (or just a
> global struct)? That way you don't have to pass it around, changing
> all those function prototypes.

I don't understand: Even if I make socklist a global structure or something like that, I also have to change all this function prototypes to remove at least the old unused parameters.

> I'm a bit intimidated by this change since I have a rather big
> patch-set on top of daemon.c, and I really don't want to resolve a lot
> of conflicts. But I guess that's my problem :P
Erik Faye-Lund· Aug 30, 2010, 12:58 UTC · re: AlexanderS · lore

Re: [PATCHv4 1/2] daemon: add helper function named_sock_setup

On Mon, Aug 30, 2010 at 2:46 PM, AlexanderS <alexander@sulfrian.net> wrote:
Show 12 quoted lines
> On Mon, 30 Aug 2010 14:12:52 +0200
> Erik Faye-Lund <kusmabite@gmail.com> wrote:
>
>> Since serve() isn't a library function, wouldn't it reduce needless
>> code churn to just make socklist a set of global variables (or just a
>> global struct)? That way you don't have to pass it around, changing
>> all those function prototypes.
>
> I don't understand: Even if I make socklist a global structure or
> something like that, I also have to change all this function prototypes
> to remove at least the old unused parameters.
>
You are of course right, strike my comment :)
Alexander Sulfrian· Aug 30, 2010, 11:30 UTC · re: Junio C Hamano · lore

[PATCHv4 2/2] daemon: allow more than one host address given via --listen

When the host has more than one interfaces, daemon can listen to all of them by not giving any --listen option, or listen to only one. Teach it to accept more than one --listen options.

Remove the hostname information form the die, if no socket could be created. It would only trigger when no interface out of either all interface or the ones specified on the command line with --listen options, can be listened to and so the user does know which "host" was asked.

Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
---
 Documentation/git-daemon.txt |    1 +
 daemon.c                     |   31 ++++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 01c9f8e..4afd0a4 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -85,6 +85,7 @@ OPTIONS
 	be either an IPv4 address or an IPv6 address if supported.  If IPv6
 	is not supported, then --listen=hostname is also not supported and
 	--listen must be given an IPv4 address.
+	Can be given more than one time.
 	Incompatible with '--inetd' option.
 
 --port=n::
diff --git a/daemon.c b/daemon.c
index c666ced..d6e20c6 100644
--- a/daemon.c
+++ b/daemon.c
@@ -3,6 +3,7 @@
 #include "exec_cmd.h"
 #include "run-command.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 #include <syslog.h>
 
@@ -866,9 +867,21 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 #endif
 
-static void socksetup(char *listen_addr, int listen_port, struct socketlist *socklist)
+static void socksetup(struct string_list *listen_addr, int listen_port, struct socketlist *socklist)
 {
-	setup_named_sock(listen_addr, listen_port, 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)
@@ -959,14 +972,14 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }
 
-static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
+static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
 {
 	struct socketlist socklist = { NULL, 0, 0 };
 
 	socksetup(listen_addr, listen_port, &socklist);
 	if (socklist.nr == 0)
-		die("unable to allocate any listen sockets on host %s port %u",
-		    listen_addr, listen_port);
+		die("unable to allocate any listen sockets on port %u",
+		    listen_port);
 
 	if (pass && gid &&
 	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
@@ -979,7 +992,7 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t
 int main(int argc, char **argv)
 {
 	int listen_port = 0;
-	char *listen_addr = NULL;
+	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
 	int inetd_mode = 0;
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
@@ -994,7 +1007,7 @@ int main(int argc, char **argv)
 		char *arg = argv[i];
 
 		if (!prefixcmp(arg, "--listen=")) {
-			listen_addr = xstrdup_tolower(arg + 9);
+			string_list_append(&listen_addr, xstrdup_tolower(arg + 9));
 			continue;
 		}
 		if (!prefixcmp(arg, "--port=")) {
@@ -1119,7 +1132,7 @@ int main(int argc, char **argv)
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
 
-	if (inetd_mode && (listen_port || listen_addr))
+	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;
@@ -1174,5 +1187,5 @@ int main(int argc, char **argv)
 	if (pid_file)
 		store_pid(pid_file);
 
-	return serve(listen_addr, listen_port, pass, gid);
+	return serve(&listen_addr, listen_port, pass, gid);
 }
-- 
1.7.1
Alexander Sulfrian· Aug 29, 2010, 15:07 UTC · re: Junio C Hamano · lore

[PATCHv2 1/2] daemon: add helper function named_sock_setup

Add named_sock_setup as helper function for socksetup to make it easier to create more than one listen sockets. named_sock_setup could be called more than one time and add the new sockets to the supplied socklist_p.

Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
---
 daemon.c |   35 ++++++++++++++++++++++++-----------
 1 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/daemon.c b/daemon.c
index e22a2b7..deda4cf 100644
--- a/daemon.c
+++ b/daemon.c
@@ -736,9 +736,9 @@ static int set_reuse_addr(int sockfd)
 
 #ifndef NO_IPV6
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p, int socknum)
 {
-	int socknum = 0, *socklist = NULL;
+	int *socklist = *socklist_p;
 	int maxfd = -1;
 	char pbuf[NI_MAXSERV];
 	struct addrinfo hints, *ai0, *ai;
@@ -810,8 +810,9 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 
 #else /* NO_IPV6 */
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p, int socknum)
 {
+	int *socklist = *socklist_p;
 	struct sockaddr_in sin;
 	int sockfd;
 	long flags;
@@ -823,41 +824,53 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 	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;
+			return socknum;
 	} else {
 		sin.sin_addr.s_addr = htonl(INADDR_ANY);
 	}
 
 	sockfd = socket(AF_INET, SOCK_STREAM, 0);
 	if (sockfd < 0)
-		return 0;
+		return socknum;
 
 	if (set_reuse_addr(sockfd)) {
 		close(sockfd);
-		return 0;
+		return socknum;
 	}
 
 	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
 		close(sockfd);
-		return 0;
+		return socknum;
 	}
 
 	if (listen(sockfd, 5) < 0) {
 		close(sockfd);
-		return 0;
+		return socknum;
 	}
 
 	flags = fcntl(sockfd, F_GETFD, 0);
 	if (flags >= 0)
 		fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
 
-	*socklist_p = xmalloc(sizeof(int));
-	**socklist_p = sockfd;
-	return 1;
+	socklist = xrealloc(socklist, sizeof(int) * (socknum + 1));
+	socklist[socknum++] = sockfd;
+
+	*socklist_p = socklist;
+	return socknum;
 }
 
 #endif
 
+static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+{
+	int socknum = 0, *socklist = NULL;
+
+	socknum = setup_named_sock(listen_addr, listen_port, &socklist, socknum);
+
+	*socklist_p = socklist;
+	return socknum;
+}
+
 static int service_loop(int socknum, int *socklist)
 {
 	struct pollfd *pfd;
-- 
1.7.1
Alexander Sulfrian· Aug 29, 2010, 15:07 UTC · re: Junio C Hamano · lore

[PATCHv2 2/2] daemon: allow more than one host address given via --listen

When the host has more than one interfaces, daemon can listen to all of them by not giving any --listen option, or listen to only one. Teach it to accept more than one --listen options.

Remove the hostname information form the die, if no socket could be created. It would only trigger when no interface out of either all interface or the ones specified on the command line with --listen options, can be listened to and so the user does know which "host" was asked.

Signed-off-by: Alexander Sulfrian <alexander@sulfrian.net>
---
 Documentation/git-daemon.txt |    1 +
 daemon.c                     |   31 ++++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 01c9f8e..4afd0a4 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -85,6 +85,7 @@ OPTIONS
 	be either an IPv4 address or an IPv6 address if supported.  If IPv6
 	is not supported, then --listen=hostname is also not supported and
 	--listen must be given an IPv4 address.
+	Can be given more than one time.
 	Incompatible with '--inetd' option.
 
 --port=n::
diff --git a/daemon.c b/daemon.c
index deda4cf..f7f7e13 100644
--- a/daemon.c
+++ b/daemon.c
@@ -3,6 +3,7 @@
 #include "exec_cmd.h"
 #include "run-command.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 #include <syslog.h>
 
@@ -861,11 +862,20 @@ static int setup_named_sock(char *listen_addr, int listen_port, int **socklist_p
 
 #endif
 
-static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
+static int socksetup(struct string_list *listen_addr, int listen_port, int **socklist_p)
 {
 	int socknum = 0, *socklist = NULL;
 
-	socknum = setup_named_sock(listen_addr, listen_port, &socklist, socknum);
+	if (!listen_addr->nr)
+		socknum = setup_named_sock(NULL, listen_port, &socklist,
+					   socknum);
+	else {
+		int i;
+		for (i = 0; i < listen_addr->nr; i++)
+			socknum = setup_named_sock(listen_addr->items[i].string,
+						   listen_port, &socklist,
+						   socknum);
+	}
 
 	*socklist_p = socklist;
 	return socknum;
@@ -959,14 +969,14 @@ static void store_pid(const char *path)
 		die_errno("failed to write pid file '%s'", path);
 }
 
-static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
+static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid)
 {
 	int socknum, *socklist;
 
 	socknum = socksetup(listen_addr, listen_port, &socklist);
 	if (socknum == 0)
-		die("unable to allocate any listen sockets on host %s port %u",
-		    listen_addr, listen_port);
+		die("unable to allocate any listen sockets on port %u",
+		    listen_port);
 
 	if (pass && gid &&
 	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
@@ -979,7 +989,7 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_t
 int main(int argc, char **argv)
 {
 	int listen_port = 0;
-	char *listen_addr = NULL;
+	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
 	int inetd_mode = 0;
 	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
@@ -987,6 +997,7 @@ int main(int argc, char **argv)
 	struct group *group;
 	gid_t gid = 0;
 	int i;
+	int return_value;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -994,7 +1005,7 @@ int main(int argc, char **argv)
 		char *arg = argv[i];
 
 		if (!prefixcmp(arg, "--listen=")) {
-			listen_addr = xstrdup_tolower(arg + 9);
+			string_list_append(&listen_addr, xstrdup_tolower(arg + 9));
 			continue;
 		}
 		if (!prefixcmp(arg, "--port=")) {
@@ -1119,7 +1130,7 @@ int main(int argc, char **argv)
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
 
-	if (inetd_mode && (listen_port || listen_addr))
+	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
 		die("--listen= and --port= are incompatible with --inetd");
 	else if (listen_port == 0)
 		listen_port = DEFAULT_GIT_PORT;
@@ -1174,5 +1185,7 @@ int main(int argc, char **argv)
 	if (pid_file)
 		store_pid(pid_file);
 
-	return serve(listen_addr, listen_port, pass, gid);
+	return_value = serve(&listen_addr, listen_port, pass, gid);
+
+	return return_value;
 }
-- 
1.7.1
Erik Faye-Lund· Aug 29, 2010, 15:11 UTC · re: Alexander Sulfrian · lore

Re: [PATCHv2 2/2] daemon: allow more than one host address given via --listen

On Sun, Aug 29, 2010 at 5:07 PM, Alexander Sulfrian <alexander@sulfrian.net> wrote:

Show 8 quoted lines
> @@ -987,6 +997,7 @@ int main(int argc, char **argv)
>        struct group *group;
>        gid_t gid = 0;
>        int i;
> +       int return_value;
>
>        git_extract_argv0_path(argv[0]);
>
<snip>
Show 9 quoted lines
> @@ -1174,5 +1185,7 @@ int main(int argc, char **argv)
>        if (pid_file)
>                store_pid(pid_file);
>
> -       return serve(listen_addr, listen_port, pass, gid);
> +       return_value = serve(&listen_addr, listen_port, pass, gid);
> +
> +       return return_value;
>  }
Uhm, why? I can't find any other uses for "return_value"...
AlexanderS· Aug 29, 2010, 15:17 UTC · re: Erik Faye-Lund · lore

Re: [PATCHv2 2/2] daemon: allow more than one host address given via --listen

On Sun, 29 Aug 2010 17:11:54 +0200 Erik Faye-Lund <kusmabite@gmail.com> wrote:

<snip>
Show 11 quoted lines
> > @@ -1174,5 +1185,7 @@ int main(int argc, char **argv)
> >        if (pid_file)
> >                store_pid(pid_file);
> >
> > -       return serve(listen_addr, listen_port, pass, gid);
> > +       return_value = serve(&listen_addr, listen_port, pass, gid);
> > +
> > +       return return_value;
> >  }
> 
> Uhm, why? I can't find any other uses for "return_value"...
See v3, I missed string_list_clear this time...

← back to recent threads