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

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

[Alexander Sulfrian]

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

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

[Alexander Sulfrian]

--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

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

[Junio C Hamano]

Alexander Sulfrian <alexander@sulfrian.net> writes:

> --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: ...

>  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.

> @@ -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.

> @@ -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.

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

[Alexander Sulfrian]

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(-)

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

[Alexander Sulfrian]

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

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

[Alexander Sulfrian]

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

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

[Erik Faye-Lund]

On Sun, Aug 29, 2010 at 5:07 PM, Alexander Sulfrian
<alexander@sulfrian.net> wrote:
> @@ -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>
> @@ -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"...

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

[Alexander Sulfrian]

Hi,
uups sorry, missed the string_list_clear in v2.

Thanks
Alex

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

[Alexander Sulfrian]

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

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

[Alexander Sulfrian]

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

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

[AlexanderS]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

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

<snip>
> > @@ -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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Junio C Hamano]

Alexander Sulfrian <alexander@sulfrian.net> writes:

> @@ -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?

> +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'".

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

[Alexander Sulfrian]

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

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

[Alexander Sulfrian]

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

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

[Alexander Sulfrian]

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

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

[Erik Faye-Lund]

On Mon, Aug 30, 2010 at 1:30 PM, Alexander Sulfrian
<alexander@sulfrian.net> wrote:
> 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

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

[AlexanderS]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Erik Faye-Lund]

On Mon, Aug 30, 2010 at 2:46 PM, AlexanderS <alexander@sulfrian.net> wrote:
> 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 :)