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

[Junio C Hamano <gitster@pobox.com>]

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.