Re: [PATCH] added possibility to supply more than one --listen argument to git-daemon
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:22
Alexander Sulfrian [off-list ref] 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 <redacted> ---
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: ...
quoted hunk
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.
quoted hunk
@@ -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.
quoted hunk
@@ -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.