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

Alexander Sulfrian <alexander@sulfrian.net> writes:

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

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.

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.

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.