[PATCH] git-daemon: SysV needs the signal handler reinstated.

[PATCH] git-daemon: SysV needs the signal handler reinstated.

[Stephen R. van den Berg]

Fixes the bug on (amongst others) Solaris that only the first
child ever is reaped.

Signed-off-by: Stephen R. van den Berg <srb@cuci.nl>
---

 daemon.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4540e8d..1c00305 100644
--- a/daemon.c
+++ b/daemon.c
@@ -794,6 +794,7 @@ static void child_handler(int signo)
 		}
 		break;
 	}
+	signal(SIGCHLD, child_handler);
 }
 
 static int set_reuse_addr(int sockfd)
@@ -947,7 +948,7 @@ static int service_loop(int socknum, int *socklist)
 	pfd[socknum].fd = child_handler_pipe[0];
 	pfd[socknum].events = POLLIN;
 
-	signal(SIGCHLD, child_handler);
+	child_handler(0);
 
 	for (;;) {
 		int i;

[PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

making the signal handler almost a no-op.
Fix the killing code to actually be smart instead of the
pseudo-random mess.
Get rid of the silly fixed array of children and make
max-connections dynamic and configurable in the process.
Make git-daemon a proper syslogging citizen with PID-info.
Simplify the overzealous double buffering in the logroutine,
remove the artificial maximum logline length in the process.

Signed-off-by: Stephen R. van den Berg <srb@cuci.nl>
---

I actually only wanted to fix the signal handler, so that it works
on SysV/Solaris systems (see previous patch).

Then again, once I started looking at the code, it's amazing how
much functionality and robustness can be *improved* by actually
removing more and more code.

 Documentation/git-daemon.txt |    8 +
 daemon.c                     |  277 +++++++++++++++---------------------------
 2 files changed, 108 insertions(+), 177 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 4ba4b75..48bcc25 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -9,8 +9,9 @@ SYNOPSIS
 --------
 [verse]
 'git daemon' [--verbose] [--syslog] [--export-all]
-	     [--timeout=n] [--init-timeout=n] [--strict-paths]
-	     [--base-path=path] [--user-path | --user-path=path]
+	     [--timeout=n] [--init-timeout=n] [--max-connections=n]
+	     [--strict-paths] [--base-path=path] [--base-path-relaxed]
+	     [--user-path | --user-path=path]
 	     [--interpolated-path=pathtemplate]
 	     [--reuseaddr] [--detach] [--pid-file=file]
 	     [--enable=service] [--disable=service]
@@ -99,6 +100,9 @@ OPTIONS
 	it takes for the server to process the sub-request and time spent
 	waiting for next client's request.
 
+--max-connections::
+	Maximum number of concurrent clients, defaults to 32.
+
 --syslog::
 	Log to syslog instead of stderr. Note that this option does not imply
 	--verbose, thus by default only error conditions will be logged.
diff --git a/daemon.c b/daemon.c
index 1c00305..8e67747 100644
--- a/daemon.c
+++ b/daemon.c
@@ -16,12 +16,11 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
-static int child_handler_pipe[2];
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
-"           [--timeout=n] [--init-timeout=n] [--strict-paths]\n"
-"           [--base-path=path] [--base-path-relaxed]\n"
+"           [--timeout=n] [--init-timeout=n] [--max-connections=n]\n"
+"           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
 "           [--interpolated-path=path]\n"
 "           [--reuseaddr] [--detach] [--pid-file=file]\n"
@@ -78,38 +77,16 @@ static struct interp interp_table[] = {
 
 static void logreport(int priority, const char *err, va_list params)
 {
-	/* We should do a single write so that it is atomic and output
-	 * of several processes do not get intermingled. */
-	char buf[1024];
-	int buflen;
-	int maxlen, msglen;
-
-	/* sizeof(buf) should be big enough for "[pid] \n" */
-	buflen = snprintf(buf, sizeof(buf), "[%ld] ", (long) getpid());
-
-	maxlen = sizeof(buf) - buflen - 1; /* -1 for our own LF */
-	msglen = vsnprintf(buf + buflen, maxlen, err, params);
-
-	if (log_syslog) {
-		syslog(priority, "%s", buf);
-		return;
+	if (log_syslog)
+		vsyslog(priority, err, params);
+	else {
+		/* Since stderr is set to linebuffered mode, the
+		 * logging of different processes will not overlap
+		 */
+		fprintf(stderr, "[%d] ", (int)getpid());
+		vfprintf(stderr, err, params);
+		fputc('\n', stderr);
 	}
-
-	/* maxlen counted our own LF but also counts space given to
-	 * vsnprintf for the terminating NUL.  We want to make sure that
-	 * we have space for our own LF and NUL after the "meat" of the
-	 * message, so truncate it at maxlen - 1.
-	 */
-	if (msglen > maxlen - 1)
-		msglen = maxlen - 1;
-	else if (msglen < 0)
-		msglen = 0; /* Protect against weird return values. */
-	buflen += msglen;
-
-	buf[buflen++] = '\n';
-	buf[buflen] = '\0';
-
-	write_in_full(2, buf, buflen);
 }
 
 static void logerror(const char *err, ...)
@@ -605,39 +582,38 @@ static int execute(struct sockaddr *addr)
 }
 
 
-/*
- * We count spawned/reaped separately, just to avoid any
- * races when updating them from signals. The SIGCHLD handler
- * will only update children_reaped, and the fork logic will
- * only update children_spawned.
- *
- * MAX_CHILDREN should be a power-of-two to make the modulus
- * operation cheap. It should also be at least twice
- * the maximum number of connections we will ever allow.
- */
-#define MAX_CHILDREN 128
-
-static int max_connections = 25;
-
-/* These are updated by the signal handler */
-static volatile unsigned int children_reaped;
-static pid_t dead_child[MAX_CHILDREN];
+static int max_connections = 32;
 
-/* These are updated by the main loop */
-static unsigned int children_spawned;
-static unsigned int children_deleted;
+static unsigned int live_children;
 
 static struct child {
+        struct child*next;
 	pid_t pid;
-	int addrlen;
 	struct sockaddr_storage address;
-} live_child[MAX_CHILDREN];
+} *firstborn;
 
-static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
+static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
 {
-	live_child[idx].pid = pid;
-	live_child[idx].addrlen = addrlen;
-	memcpy(&live_child[idx].address, addr, addrlen);
+        struct child*newborn;
+        newborn = xmalloc(sizeof *newborn);
+        if (newborn) {
+	        struct child**cradle,*blanket;
+
+		live_children++;
+		newborn->pid = pid;
+		memcpy(memset(&newborn->address, 0, sizeof newborn->address),
+		 addr, addrlen);
+		for (cradle = &firstborn;
+		     (blanket = *cradle);
+		     cradle = &blanket->next)
+			if (!memcmp(&blanket->address, &newborn->address,
+				   sizeof newborn->address))
+				break;
+		newborn->next = blanket;
+		*cradle = newborn;
+        }
+	else
+		logerror("Out of memory spawning new child");
 }
 
 /*
@@ -646,107 +622,74 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
  * We move everything up by one, since the new "deleted" will
  * be one higher.
  */
-static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)
+static void remove_child(pid_t pid)
 {
-	struct child n;
-
-	deleted %= MAX_CHILDREN;
-	spawned %= MAX_CHILDREN;
-	if (live_child[deleted].pid == pid) {
-		live_child[deleted].pid = -1;
-		return;
-	}
-	n = live_child[deleted];
-	for (;;) {
-		struct child m;
-		deleted = (deleted + 1) % MAX_CHILDREN;
-		if (deleted == spawned)
-			die("could not find dead child %d\n", pid);
-		m = live_child[deleted];
-		live_child[deleted] = n;
-		if (m.pid == pid)
-			return;
-		n = m;
-	}
+	struct child**cradle,*blanket;
+
+	for (cradle = &firstborn;
+	     (blanket = *cradle);
+	     cradle = &blanket->next)
+		if (blanket->pid == pid) {
+			*cradle = blanket->next;
+			live_children--;
+			free(blanket);
+			break;
+		}
 }
 
 /*
  * This gets called if the number of connections grows
  * past "max_connections".
  *
- * We _should_ start off by searching for connections
- * from the same IP, and if there is some address wth
- * multiple connections, we should kill that first.
- *
- * As it is, we just "randomly" kill 25% of the connections,
- * and our pseudo-random generator sucks too. I have no
- * shame.
- *
- * Really, this is just a place-holder for a _real_ algorithm.
+ * We kill the newest connection from a duplicate IP first.
+ * If there are no duplicates, then the newest connection is killed,
+ * in order to allow long running clones to actually complete.
  */
-static void kill_some_children(int signo, unsigned start, unsigned stop)
+static void kill_some_child(int signo)
 {
-	start %= MAX_CHILDREN;
-	stop %= MAX_CHILDREN;
-	while (start != stop) {
-		if (!(start & 3))
-			kill(live_child[start].pid, signo);
-		start = (start + 1) % MAX_CHILDREN;
+	const struct child *blanket;
+
+	if ((blanket = firstborn)) {
+		const struct child *next;
+
+		for (; (next = blanket->next); blanket = next)
+			if (!memcmp(&blanket->address, &next->address,
+				   sizeof next->address))
+				break;
+
+		kill(blanket->pid, signo);
 	}
 }
 
 static void check_dead_children(void)
 {
-	unsigned spawned, reaped, deleted;
-
-	spawned = children_spawned;
-	reaped = children_reaped;
-	deleted = children_deleted;
-
-	while (deleted < reaped) {
-		pid_t pid = dead_child[deleted % MAX_CHILDREN];
-		const char *dead = pid < 0 ? " (with error)" : "";
-
-		if (pid < 0)
-			pid = -pid;
+	int status;
+	pid_t pid;
 
-		/* XXX: Custom logging, since we don't wanna getpid() */
-		if (verbose) {
-			if (log_syslog)
-				syslog(LOG_INFO, "[%d] Disconnected%s",
-						pid, dead);
-			else
-				fprintf(stderr, "[%d] Disconnected%s\n",
-						pid, dead);
-		}
-		remove_child(pid, deleted, spawned);
-		deleted++;
+	while ((pid = waitpid(-1, &status, WNOHANG))>0) {
+		const char *dead = "";
+		remove_child(pid);
+		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
+			dead = " (with error)";
+		loginfo("[%d] Disconnected%s", (int)pid, dead);
 	}
-	children_deleted = deleted;
 }
 
 static void check_max_connections(void)
 {
 	for (;;) {
-		int active;
-		unsigned spawned, deleted;
-
 		check_dead_children();
-
-		spawned = children_spawned;
-		deleted = children_deleted;
-
-		active = spawned - deleted;
-		if (active <= max_connections)
+		if (live_children <= max_connections)
 			break;
 
 		/* Kill some unstarted connections with SIGTERM */
-		kill_some_children(SIGTERM, deleted, spawned);
-		if (active <= max_connections << 1)
+		kill_some_child(SIGTERM);
+		check_dead_children();
+		if (live_children <= max_connections << 1)
 			break;
 
 		/* If the SIGTERM thing isn't helping use SIGKILL */
-		kill_some_children(SIGKILL, deleted, spawned);
+		kill_some_child(SIGKILL);
 		sleep(1);
 	}
 }
@@ -756,16 +699,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 	pid_t pid = fork();
 
 	if (pid) {
-		unsigned idx;
-
 		close(incoming);
-		if (pid < 0)
+		if (pid < 0) {
+			logerror("Couldn't fork %s", strerror(errno));
 			return;
+		}
 
-		idx = children_spawned % MAX_CHILDREN;
-		children_spawned++;
-		add_child(idx, pid, addr, addrlen);
-
+		add_child(pid, addr, addrlen);
 		check_max_connections();
 		return;
 	}
@@ -779,21 +719,10 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 
 static void child_handler(int signo)
 {
-	for (;;) {
-		int status;
-		pid_t pid = waitpid(-1, &status, WNOHANG);
-
-		if (pid > 0) {
-			unsigned reaped = children_reaped;
-			if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
-				pid = -pid;
-			dead_child[reaped % MAX_CHILDREN] = pid;
-			children_reaped = reaped + 1;
-			write(child_handler_pipe[1], &status, 1);
-			continue;
-		}
-		break;
-	}
+	/* Otherwise empty handler because systemcalls will get interrupted
+         * upon signal receipt
+         * SysV needs the handler to be reinstated
+         */
 	signal(SIGCHLD, child_handler);
 }
 
@@ -936,35 +865,30 @@ static int service_loop(int socknum, int *socklist)
 	struct pollfd *pfd;
 	int i;
 
-	if (pipe(child_handler_pipe) < 0)
-		die ("Could not set up pipe for child handler");
-
-	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
+	pfd = xcalloc(socknum, sizeof(struct pollfd));
 
 	for (i = 0; i < socknum; i++) {
 		pfd[i].fd = socklist[i];
 		pfd[i].events = POLLIN;
 	}
-	pfd[socknum].fd = child_handler_pipe[0];
-	pfd[socknum].events = POLLIN;
-
-	child_handler(0);
 
 	for (;;) {
 		int i;
+		int olderrno;
+
+		i = poll(pfd, socknum, -1);
+		olderrno = errno;
 
-		if (poll(pfd, socknum + 1, -1) < 0) {
-			if (errno != EINTR) {
+		check_dead_children();
+
+		if (i < 0) {
+			if (olderrno != EINTR) {
 				error("poll failed, resuming: %s",
-				      strerror(errno));
+				      strerror(olderrno));
 				sleep(1);
 			}
 			continue;
 		}
-		if (pfd[socknum].revents & POLLIN) {
-			read(child_handler_pipe[0], &i, 1);
-			check_dead_children();
-		}
 
 		for (i = 0; i < socknum; i++) {
 			if (pfd[i].revents & POLLIN) {
@@ -1055,10 +979,7 @@ int main(int argc, char **argv)
 	gid_t gid = 0;
 	int i;
 
-	/* Without this we cannot rely on waitpid() to tell
-	 * what happened to our children.
-	 */
-	signal(SIGCHLD, SIG_DFL);
+	child_handler(0);
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
@@ -1105,6 +1026,10 @@ int main(int argc, char **argv)
 			init_timeout = atoi(arg+15);
 			continue;
 		}
+		if (!prefixcmp(arg, "--max-connections=")) {
+			max_connections = atoi(arg+18);
+			continue;
+		}
 		if (!strcmp(arg, "--strict-paths")) {
 			strict_paths = 1;
 			continue;
@@ -1178,9 +1103,11 @@ int main(int argc, char **argv)
 	}
 
 	if (log_syslog) {
-		openlog("git-daemon", 0, LOG_DAEMON);
+		openlog("git-daemon", LOG_PID, LOG_DAEMON);
 		set_die_routine(daemon_die);
 	}
+	else			    /* so that logging into a file is atomic */
+		setlinebuf(stderr);
 
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Junio C Hamano]

"Stephen R. van den Berg" <srb@cuci.nl> writes:

> making the signal handler almost a no-op.
> Fix the killing code to actually be smart instead of the
> pseudo-random mess.
> Get rid of the silly fixed array of children and make
> max-connections dynamic and configurable in the process.
> Make git-daemon a proper syslogging citizen with PID-info.
> Simplify the overzealous double buffering in the logroutine,
> remove the artificial maximum logline length in the process.
>
> Signed-off-by: Stephen R. van den Berg <srb@cuci.nl>

Sorry, but this does too many things in one patch.

 - Taking advantage of poll() getting interrupted by SIGCHLD, so that you
   do not have to do anything in the signal handler, is so obvious that I
   am actually ashamed of not having to think of it the last time we
   touched this code.  Is there a poll() that does not return EINTR but
   just call the handler and restart after that as if nothing has
   happened, I have to wonder...

 - Conversion from silly fixed array to dynamic and configurable maximum
   would be a good idea, but that is independent from the above, isn't it?

 - I see you have a call to vsyslog, which is the first user of the
   function.  How portable is it (the patch coming from you, I know
   Solaris would have it, and recent 4BSD also would, but what about the
   others)?

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

Junio C Hamano wrote:
>"Stephen R. van den Berg" <srb@cuci.nl> writes:
>Sorry, but this does too many things in one patch.

Yes, I know, got carried away.  Then again, the code has a lot of
overlapping places (spacewise); I kind of leapt from one place to the
next; you fix one thing, and then the next wart stares you in the face.
I'll see if I can split it up, if that suits you better.

> - Taking advantage of poll() getting interrupted by SIGCHLD, so that you
>   do not have to do anything in the signal handler, is so obvious that I
>   am actually ashamed of not having to think of it the last time we
>   touched this code.  Is there a poll() that does not return EINTR but
>   just call the handler and restart after that as if nothing has
>   happened, I have to wonder...

Only if the signal is set to SIG_IGN on all systems I worked with since
1987.

> - Conversion from silly fixed array to dynamic and configurable maximum
>   would be a good idea, but that is independent from the above, isn't it?

It is, but the code is on the same lines (in large parts).
Separating it causes two things:
a. The patches to become dependent on each other in the timeline.
b. More (redundant) work, because some parts that need to be rewritten, get
   deleted by the following patch(es).

> - I see you have a call to vsyslog, which is the first user of the
>   function.  How portable is it (the patch coming from you, I know
>   Solaris would have it, and recent 4BSD also would, but what about the
>   others)?

Cygwin has it, Solaris does, Linux does, MacOSX does.
AIX and HPUX don't, perhaps.
I'll see what I can do to avoid it, yet simplify the code.
-- 
Sincerely,
           Stephen R. van den Berg.

Father's Day Special at the local clinic -- Vasectomy!

[PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

making the signal handler almost a no-op.
Fix the killing code to actually be smart instead of the
pseudo-random mess.
Get rid of the silly fixed array of children and make
max-connections dynamic and configurable in the process.
Make git-daemon a proper syslogging citizen with PID-info.
Simplify the overzealous double buffering in the logroutine,
remove the artificial maximum logline length in the process.
Changed two calls to error() into logerror().

Signed-off-by: Stephen R. van den Berg <srb@cuci.nl>
---

Took out the vsyslog().
Cleanup a bit left and right.
Can this go in in one piece?  Or do I need to split it up?
Like I said, large parts of the split-up are obsolete due do getting
deleted.

 Documentation/git-daemon.txt |    8 +
 daemon.c                     |  285 ++++++++++++++++--------------------------
 2 files changed, 114 insertions(+), 179 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 4ba4b75..48bcc25 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -9,8 +9,9 @@ SYNOPSIS
 --------
 [verse]
 'git daemon' [--verbose] [--syslog] [--export-all]
-	     [--timeout=n] [--init-timeout=n] [--strict-paths]
-	     [--base-path=path] [--user-path | --user-path=path]
+	     [--timeout=n] [--init-timeout=n] [--max-connections=n]
+	     [--strict-paths] [--base-path=path] [--base-path-relaxed]
+	     [--user-path | --user-path=path]
 	     [--interpolated-path=pathtemplate]
 	     [--reuseaddr] [--detach] [--pid-file=file]
 	     [--enable=service] [--disable=service]
@@ -99,6 +100,9 @@ OPTIONS
 	it takes for the server to process the sub-request and time spent
 	waiting for next client's request.
 
+--max-connections::
+	Maximum number of concurrent clients, defaults to 32.
+
 --syslog::
 	Log to syslog instead of stderr. Note that this option does not imply
 	--verbose, thus by default only error conditions will be logged.
diff --git a/daemon.c b/daemon.c
index 1c00305..82eb224 100644
--- a/daemon.c
+++ b/daemon.c
@@ -16,12 +16,11 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
-static int child_handler_pipe[2];
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
-"           [--timeout=n] [--init-timeout=n] [--strict-paths]\n"
-"           [--base-path=path] [--base-path-relaxed]\n"
+"           [--timeout=n] [--init-timeout=n] [--max-connections=n]\n"
+"           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
 "           [--interpolated-path=path]\n"
 "           [--reuseaddr] [--detach] [--pid-file=file]\n"
@@ -78,38 +77,19 @@ static struct interp interp_table[] = {
 
 static void logreport(int priority, const char *err, va_list params)
 {
-	/* We should do a single write so that it is atomic and output
-	 * of several processes do not get intermingled. */
-	char buf[1024];
-	int buflen;
-	int maxlen, msglen;
-
-	/* sizeof(buf) should be big enough for "[pid] \n" */
-	buflen = snprintf(buf, sizeof(buf), "[%ld] ", (long) getpid());
-
-	maxlen = sizeof(buf) - buflen - 1; /* -1 for our own LF */
-	msglen = vsnprintf(buf + buflen, maxlen, err, params);
-
 	if (log_syslog) {
+		char buf[1024];
+		vsnprintf(buf, sizeof(buf), err, params);
 		syslog(priority, "%s", buf);
-		return;
 	}
-
-	/* maxlen counted our own LF but also counts space given to
-	 * vsnprintf for the terminating NUL.  We want to make sure that
-	 * we have space for our own LF and NUL after the "meat" of the
-	 * message, so truncate it at maxlen - 1.
-	 */
-	if (msglen > maxlen - 1)
-		msglen = maxlen - 1;
-	else if (msglen < 0)
-		msglen = 0; /* Protect against weird return values. */
-	buflen += msglen;
-
-	buf[buflen++] = '\n';
-	buf[buflen] = '\0';
-
-	write_in_full(2, buf, buflen);
+	else {
+		/* Since stderr is set to linebuffered mode, the
+		 * logging of different processes will not overlap
+		 */
+		fprintf(stderr, "[%d] ", (int)getpid());
+		vfprintf(stderr, err, params);
+		fputc('\n', stderr);
+	}
 }
 
 static void logerror(const char *err, ...)
@@ -604,40 +584,38 @@ static int execute(struct sockaddr *addr)
 	return -1;
 }
 
+static int max_connections = 32;
 
-/*
- * We count spawned/reaped separately, just to avoid any
- * races when updating them from signals. The SIGCHLD handler
- * will only update children_reaped, and the fork logic will
- * only update children_spawned.
- *
- * MAX_CHILDREN should be a power-of-two to make the modulus
- * operation cheap. It should also be at least twice
- * the maximum number of connections we will ever allow.
- */
-#define MAX_CHILDREN 128
-
-static int max_connections = 25;
-
-/* These are updated by the signal handler */
-static volatile unsigned int children_reaped;
-static pid_t dead_child[MAX_CHILDREN];
-
-/* These are updated by the main loop */
-static unsigned int children_spawned;
-static unsigned int children_deleted;
+static unsigned int live_children;
 
 static struct child {
+	struct child*next;
 	pid_t pid;
-	int addrlen;
 	struct sockaddr_storage address;
-} live_child[MAX_CHILDREN];
+} *firstborn;
 
-static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
+static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
 {
-	live_child[idx].pid = pid;
-	live_child[idx].addrlen = addrlen;
-	memcpy(&live_child[idx].address, addr, addrlen);
+	struct child*newborn;
+	newborn = xmalloc(sizeof *newborn);
+	if (newborn) {
+		struct child **cradle, *blanket;
+
+		live_children++;
+		newborn->pid = pid;
+		memcpy(memset(&newborn->address, 0, sizeof newborn->address),
+		 addr, addrlen);
+		for (cradle = &firstborn;
+		     (blanket = *cradle);
+		     cradle = &blanket->next)
+			if (!memcmp(&blanket->address, &newborn->address,
+				   sizeof newborn->address))
+				break;
+		newborn->next = blanket;
+		*cradle = newborn;
+	}
+	else
+		logerror("Out of memory spawning new child");
 }
 
 /*
@@ -646,107 +624,74 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
  * We move everything up by one, since the new "deleted" will
  * be one higher.
  */
-static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)
+static void remove_child(pid_t pid)
 {
-	struct child n;
-
-	deleted %= MAX_CHILDREN;
-	spawned %= MAX_CHILDREN;
-	if (live_child[deleted].pid == pid) {
-		live_child[deleted].pid = -1;
-		return;
-	}
-	n = live_child[deleted];
-	for (;;) {
-		struct child m;
-		deleted = (deleted + 1) % MAX_CHILDREN;
-		if (deleted == spawned)
-			die("could not find dead child %d\n", pid);
-		m = live_child[deleted];
-		live_child[deleted] = n;
-		if (m.pid == pid)
-			return;
-		n = m;
-	}
+	struct child **cradle, *blanket;
+
+	for (cradle = &firstborn;
+	     (blanket = *cradle);
+	     cradle = &blanket->next)
+		if (blanket->pid == pid) {
+			*cradle = blanket->next;
+			live_children--;
+			free(blanket);
+			break;
+		}
 }
 
 /*
  * This gets called if the number of connections grows
  * past "max_connections".
  *
- * We _should_ start off by searching for connections
- * from the same IP, and if there is some address wth
- * multiple connections, we should kill that first.
- *
- * As it is, we just "randomly" kill 25% of the connections,
- * and our pseudo-random generator sucks too. I have no
- * shame.
- *
- * Really, this is just a place-holder for a _real_ algorithm.
+ * We kill the newest connection from a duplicate IP first.
+ * If there are no duplicates, then the newest connection is killed,
+ * in order to allow long running clones to actually complete.
  */
-static void kill_some_children(int signo, unsigned start, unsigned stop)
+static void kill_some_child(int signo)
 {
-	start %= MAX_CHILDREN;
-	stop %= MAX_CHILDREN;
-	while (start != stop) {
-		if (!(start & 3))
-			kill(live_child[start].pid, signo);
-		start = (start + 1) % MAX_CHILDREN;
+	const struct child *blanket;
+
+	if ((blanket = firstborn)) {
+		const struct child *next;
+
+		for (; (next = blanket->next); blanket = next)
+			if (!memcmp(&blanket->address, &next->address,
+				   sizeof next->address))
+				break;
+
+		kill(blanket->pid, signo);
 	}
 }
 
 static void check_dead_children(void)
 {
-	unsigned spawned, reaped, deleted;
-
-	spawned = children_spawned;
-	reaped = children_reaped;
-	deleted = children_deleted;
-
-	while (deleted < reaped) {
-		pid_t pid = dead_child[deleted % MAX_CHILDREN];
-		const char *dead = pid < 0 ? " (with error)" : "";
-
-		if (pid < 0)
-			pid = -pid;
+	int status;
+	pid_t pid;
 
-		/* XXX: Custom logging, since we don't wanna getpid() */
-		if (verbose) {
-			if (log_syslog)
-				syslog(LOG_INFO, "[%d] Disconnected%s",
-						pid, dead);
-			else
-				fprintf(stderr, "[%d] Disconnected%s\n",
-						pid, dead);
-		}
-		remove_child(pid, deleted, spawned);
-		deleted++;
+	while ((pid = waitpid(-1, &status, WNOHANG))>0) {
+		const char *dead = "";
+		remove_child(pid);
+		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
+			dead = " (with error)";
+		loginfo("[%d] Disconnected%s", (int)pid, dead);
 	}
-	children_deleted = deleted;
 }
 
 static void check_max_connections(void)
 {
 	for (;;) {
-		int active;
-		unsigned spawned, deleted;
-
 		check_dead_children();
-
-		spawned = children_spawned;
-		deleted = children_deleted;
-
-		active = spawned - deleted;
-		if (active <= max_connections)
+		if (live_children <= max_connections)
 			break;
 
 		/* Kill some unstarted connections with SIGTERM */
-		kill_some_children(SIGTERM, deleted, spawned);
-		if (active <= max_connections << 1)
+		kill_some_child(SIGTERM);
+		check_dead_children();
+		if (live_children <= max_connections << 1)
 			break;
 
 		/* If the SIGTERM thing isn't helping use SIGKILL */
-		kill_some_children(SIGKILL, deleted, spawned);
+		kill_some_child(SIGKILL);
 		sleep(1);
 	}
 }
@@ -756,16 +701,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 	pid_t pid = fork();
 
 	if (pid) {
-		unsigned idx;
-
 		close(incoming);
-		if (pid < 0)
+		if (pid < 0) {
+			logerror("Couldn't fork %s", strerror(errno));
 			return;
+		}
 
-		idx = children_spawned % MAX_CHILDREN;
-		children_spawned++;
-		add_child(idx, pid, addr, addrlen);
-
+		add_child(pid, addr, addrlen);
 		check_max_connections();
 		return;
 	}
@@ -779,21 +721,10 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 
 static void child_handler(int signo)
 {
-	for (;;) {
-		int status;
-		pid_t pid = waitpid(-1, &status, WNOHANG);
-
-		if (pid > 0) {
-			unsigned reaped = children_reaped;
-			if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
-				pid = -pid;
-			dead_child[reaped % MAX_CHILDREN] = pid;
-			children_reaped = reaped + 1;
-			write(child_handler_pipe[1], &status, 1);
-			continue;
-		}
-		break;
-	}
+	/* Otherwise empty handler because systemcalls will get interrupted
+	 * upon signal receipt
+	 * SysV needs the handler to be reinstated
+	 */
 	signal(SIGCHLD, child_handler);
 }
 
@@ -836,7 +767,7 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 		if (sockfd < 0)
 			continue;
 		if (sockfd >= FD_SETSIZE) {
-			error("too large socket descriptor.");
+			logerror("too large socket descriptor.");
 			close(sockfd);
 			continue;
 		}
@@ -936,35 +867,30 @@ static int service_loop(int socknum, int *socklist)
 	struct pollfd *pfd;
 	int i;
 
-	if (pipe(child_handler_pipe) < 0)
-		die ("Could not set up pipe for child handler");
-
-	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
+	pfd = xcalloc(socknum, sizeof(struct pollfd));
 
 	for (i = 0; i < socknum; i++) {
 		pfd[i].fd = socklist[i];
 		pfd[i].events = POLLIN;
 	}
-	pfd[socknum].fd = child_handler_pipe[0];
-	pfd[socknum].events = POLLIN;
-
-	child_handler(0);
 
 	for (;;) {
 		int i;
+		int olderrno;
+
+		i = poll(pfd, socknum, -1);
+		olderrno = errno;
 
-		if (poll(pfd, socknum + 1, -1) < 0) {
-			if (errno != EINTR) {
-				error("poll failed, resuming: %s",
-				      strerror(errno));
+		check_dead_children();
+
+		if (i < 0) {
+			if (olderrno != EINTR) {
+				logerror("poll failed, resuming: %s",
+				      strerror(olderrno));
 				sleep(1);
 			}
 			continue;
 		}
-		if (pfd[socknum].revents & POLLIN) {
-			read(child_handler_pipe[0], &i, 1);
-			check_dead_children();
-		}
 
 		for (i = 0; i < socknum; i++) {
 			if (pfd[i].revents & POLLIN) {
@@ -1055,10 +981,7 @@ int main(int argc, char **argv)
 	gid_t gid = 0;
 	int i;
 
-	/* Without this we cannot rely on waitpid() to tell
-	 * what happened to our children.
-	 */
-	signal(SIGCHLD, SIG_DFL);
+	child_handler(0);
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
@@ -1105,6 +1028,10 @@ int main(int argc, char **argv)
 			init_timeout = atoi(arg+15);
 			continue;
 		}
+		if (!prefixcmp(arg, "--max-connections=")) {
+			max_connections = atoi(arg+18);
+			continue;
+		}
 		if (!strcmp(arg, "--strict-paths")) {
 			strict_paths = 1;
 			continue;
@@ -1178,9 +1105,11 @@ int main(int argc, char **argv)
 	}
 
 	if (log_syslog) {
-		openlog("git-daemon", 0, LOG_DAEMON);
+		openlog("git-daemon", LOG_PID, LOG_DAEMON);
 		set_die_routine(daemon_die);
 	}
+	else			    /* so that logging into a file is atomic */
+		setlinebuf(stderr);
 
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
@@ -1233,8 +1162,10 @@ int main(int argc, char **argv)
 		return execute(peer);
 	}
 
-	if (detach)
+	if (detach) {
 		daemonize();
+		loginfo("Ready to rumble");
+	}
 	else
 		sanitize_stdfds();
 

[PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

making the signal handler almost a no-op.
Fix the killing code to actually be smart instead of the
pseudo-random mess.
Get rid of the silly fixed array of children and make
max-connections dynamic and configurable in the process.
Make git-daemon a proper syslogging citizen with PID-info.
Simplify the overzealous double buffering in the logroutine,
remove the artificial maximum logline length in the process.
Changed two calls to error() into logerror().

Signed-off-by: Stephen R. van den Berg <srb@cuci.nl>
---

Cleaned up a bit further.

 Documentation/git-daemon.txt |    8 +
 daemon.c                     |  280 ++++++++++++++++--------------------------
 2 files changed, 110 insertions(+), 178 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 4ba4b75..48bcc25 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -9,8 +9,9 @@ SYNOPSIS
 --------
 [verse]
 'git daemon' [--verbose] [--syslog] [--export-all]
-	     [--timeout=n] [--init-timeout=n] [--strict-paths]
-	     [--base-path=path] [--user-path | --user-path=path]
+	     [--timeout=n] [--init-timeout=n] [--max-connections=n]
+	     [--strict-paths] [--base-path=path] [--base-path-relaxed]
+	     [--user-path | --user-path=path]
 	     [--interpolated-path=pathtemplate]
 	     [--reuseaddr] [--detach] [--pid-file=file]
 	     [--enable=service] [--disable=service]
@@ -99,6 +100,9 @@ OPTIONS
 	it takes for the server to process the sub-request and time spent
 	waiting for next client's request.
 
+--max-connections::
+	Maximum number of concurrent clients, defaults to 32.
+
 --syslog::
 	Log to syslog instead of stderr. Note that this option does not imply
 	--verbose, thus by default only error conditions will be logged.
diff --git a/daemon.c b/daemon.c
index 1c00305..0de92a8 100644
--- a/daemon.c
+++ b/daemon.c
@@ -16,12 +16,11 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
-static int child_handler_pipe[2];
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
-"           [--timeout=n] [--init-timeout=n] [--strict-paths]\n"
-"           [--base-path=path] [--base-path-relaxed]\n"
+"           [--timeout=n] [--init-timeout=n] [--max-connections=n]\n"
+"           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
 "           [--interpolated-path=path]\n"
 "           [--reuseaddr] [--detach] [--pid-file=file]\n"
@@ -78,38 +77,19 @@ static struct interp interp_table[] = {
 
 static void logreport(int priority, const char *err, va_list params)
 {
-	/* We should do a single write so that it is atomic and output
-	 * of several processes do not get intermingled. */
-	char buf[1024];
-	int buflen;
-	int maxlen, msglen;
-
-	/* sizeof(buf) should be big enough for "[pid] \n" */
-	buflen = snprintf(buf, sizeof(buf), "[%ld] ", (long) getpid());
-
-	maxlen = sizeof(buf) - buflen - 1; /* -1 for our own LF */
-	msglen = vsnprintf(buf + buflen, maxlen, err, params);
-
 	if (log_syslog) {
+		char buf[1024];
+		vsnprintf(buf, sizeof(buf), err, params);
 		syslog(priority, "%s", buf);
-		return;
 	}
-
-	/* maxlen counted our own LF but also counts space given to
-	 * vsnprintf for the terminating NUL.  We want to make sure that
-	 * we have space for our own LF and NUL after the "meat" of the
-	 * message, so truncate it at maxlen - 1.
-	 */
-	if (msglen > maxlen - 1)
-		msglen = maxlen - 1;
-	else if (msglen < 0)
-		msglen = 0; /* Protect against weird return values. */
-	buflen += msglen;
-
-	buf[buflen++] = '\n';
-	buf[buflen] = '\0';
-
-	write_in_full(2, buf, buflen);
+	else {
+		/* Since stderr is set to linebuffered mode, the
+		 * logging of different processes will not overlap
+		 */
+		fprintf(stderr, "[%d] ", (int)getpid());
+		vfprintf(stderr, err, params);
+		fputc('\n', stderr);
+	}
 }
 
 static void logerror(const char *err, ...)
@@ -604,40 +584,37 @@ static int execute(struct sockaddr *addr)
 	return -1;
 }
 
+static int max_connections = 32;
 
-/*
- * We count spawned/reaped separately, just to avoid any
- * races when updating them from signals. The SIGCHLD handler
- * will only update children_reaped, and the fork logic will
- * only update children_spawned.
- *
- * MAX_CHILDREN should be a power-of-two to make the modulus
- * operation cheap. It should also be at least twice
- * the maximum number of connections we will ever allow.
- */
-#define MAX_CHILDREN 128
-
-static int max_connections = 25;
-
-/* These are updated by the signal handler */
-static volatile unsigned int children_reaped;
-static pid_t dead_child[MAX_CHILDREN];
-
-/* These are updated by the main loop */
-static unsigned int children_spawned;
-static unsigned int children_deleted;
+static unsigned int live_children;
 
 static struct child {
+	struct child*next;
 	pid_t pid;
-	int addrlen;
 	struct sockaddr_storage address;
-} live_child[MAX_CHILDREN];
+} *firstborn;
 
-static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
+static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
 {
-	live_child[idx].pid = pid;
-	live_child[idx].addrlen = addrlen;
-	memcpy(&live_child[idx].address, addr, addrlen);
+	struct child*newborn;
+	newborn = xcalloc(1, sizeof *newborn);
+	if (newborn) {
+		struct child **cradle, *blanket;
+
+		live_children++;
+		newborn->pid = pid;
+		memcpy(&newborn->address, addr, addrlen);
+		for (cradle = &firstborn;
+		     (blanket = *cradle);
+		     cradle = &blanket->next)
+			if (!memcmp(&blanket->address, &newborn->address,
+				   sizeof newborn->address))
+				break;
+		newborn->next = blanket;
+		*cradle = newborn;
+	}
+	else
+		logerror("Out of memory spawning new child");
 }
 
 /*
@@ -646,107 +623,72 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
  * We move everything up by one, since the new "deleted" will
  * be one higher.
  */
-static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)
+static void remove_child(pid_t pid)
 {
-	struct child n;
+	struct child **cradle, *blanket;
 
-	deleted %= MAX_CHILDREN;
-	spawned %= MAX_CHILDREN;
-	if (live_child[deleted].pid == pid) {
-		live_child[deleted].pid = -1;
-		return;
-	}
-	n = live_child[deleted];
-	for (;;) {
-		struct child m;
-		deleted = (deleted + 1) % MAX_CHILDREN;
-		if (deleted == spawned)
-			die("could not find dead child %d\n", pid);
-		m = live_child[deleted];
-		live_child[deleted] = n;
-		if (m.pid == pid)
-			return;
-		n = m;
-	}
+	for (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)
+		if (blanket->pid == pid) {
+			*cradle = blanket->next;
+			live_children--;
+			free(blanket);
+			break;
+		}
 }
 
 /*
  * This gets called if the number of connections grows
  * past "max_connections".
  *
- * We _should_ start off by searching for connections
- * from the same IP, and if there is some address wth
- * multiple connections, we should kill that first.
- *
- * As it is, we just "randomly" kill 25% of the connections,
- * and our pseudo-random generator sucks too. I have no
- * shame.
- *
- * Really, this is just a place-holder for a _real_ algorithm.
+ * We kill the newest connection from a duplicate IP first.
+ * If there are no duplicates, then the newest connection is killed,
+ * in order to allow long running clones to actually complete.
  */
-static void kill_some_children(int signo, unsigned start, unsigned stop)
+static void kill_some_child(int signo)
 {
-	start %= MAX_CHILDREN;
-	stop %= MAX_CHILDREN;
-	while (start != stop) {
-		if (!(start & 3))
-			kill(live_child[start].pid, signo);
-		start = (start + 1) % MAX_CHILDREN;
+	const struct child *blanket;
+
+	if ((blanket = firstborn)) {
+		const struct child *next;
+
+		for (; (next = blanket->next); blanket = next)
+			if (!memcmp(&blanket->address, &next->address,
+				   sizeof next->address))
+				break;
+
+		kill(blanket->pid, signo);
 	}
 }
 
 static void check_dead_children(void)
 {
-	unsigned spawned, reaped, deleted;
-
-	spawned = children_spawned;
-	reaped = children_reaped;
-	deleted = children_deleted;
-
-	while (deleted < reaped) {
-		pid_t pid = dead_child[deleted % MAX_CHILDREN];
-		const char *dead = pid < 0 ? " (with error)" : "";
-
-		if (pid < 0)
-			pid = -pid;
+	int status;
+	pid_t pid;
 
-		/* XXX: Custom logging, since we don't wanna getpid() */
-		if (verbose) {
-			if (log_syslog)
-				syslog(LOG_INFO, "[%d] Disconnected%s",
-						pid, dead);
-			else
-				fprintf(stderr, "[%d] Disconnected%s\n",
-						pid, dead);
-		}
-		remove_child(pid, deleted, spawned);
-		deleted++;
+	while ((pid = waitpid(-1, &status, WNOHANG))>0) {
+		const char *dead = "";
+		remove_child(pid);
+		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
+			dead = " (with error)";
+		loginfo("[%d] Disconnected%s", (int)pid, dead);
 	}
-	children_deleted = deleted;
 }
 
 static void check_max_connections(void)
 {
 	for (;;) {
-		int active;
-		unsigned spawned, deleted;
-
 		check_dead_children();
-
-		spawned = children_spawned;
-		deleted = children_deleted;
-
-		active = spawned - deleted;
-		if (active <= max_connections)
+		if (live_children <= max_connections)
 			break;
 
 		/* Kill some unstarted connections with SIGTERM */
-		kill_some_children(SIGTERM, deleted, spawned);
-		if (active <= max_connections << 1)
+		kill_some_child(SIGTERM);
+		check_dead_children();
+		if (live_children <= max_connections << 1)
 			break;
 
 		/* If the SIGTERM thing isn't helping use SIGKILL */
-		kill_some_children(SIGKILL, deleted, spawned);
+		kill_some_child(SIGKILL);
 		sleep(1);
 	}
 }
@@ -756,16 +698,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 	pid_t pid = fork();
 
 	if (pid) {
-		unsigned idx;
-
 		close(incoming);
-		if (pid < 0)
+		if (pid < 0) {
+			logerror("Couldn't fork %s", strerror(errno));
 			return;
+		}
 
-		idx = children_spawned % MAX_CHILDREN;
-		children_spawned++;
-		add_child(idx, pid, addr, addrlen);
-
+		add_child(pid, addr, addrlen);
 		check_max_connections();
 		return;
 	}
@@ -779,21 +718,10 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
 
 static void child_handler(int signo)
 {
-	for (;;) {
-		int status;
-		pid_t pid = waitpid(-1, &status, WNOHANG);
-
-		if (pid > 0) {
-			unsigned reaped = children_reaped;
-			if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
-				pid = -pid;
-			dead_child[reaped % MAX_CHILDREN] = pid;
-			children_reaped = reaped + 1;
-			write(child_handler_pipe[1], &status, 1);
-			continue;
-		}
-		break;
-	}
+	/* Otherwise empty handler because systemcalls will get interrupted
+	 * upon signal receipt
+	 * SysV needs the handler to be reinstated
+	 */
 	signal(SIGCHLD, child_handler);
 }
 
@@ -836,7 +764,7 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
 		if (sockfd < 0)
 			continue;
 		if (sockfd >= FD_SETSIZE) {
-			error("too large socket descriptor.");
+			logerror("too large socket descriptor.");
 			close(sockfd);
 			continue;
 		}
@@ -936,35 +864,30 @@ static int service_loop(int socknum, int *socklist)
 	struct pollfd *pfd;
 	int i;
 
-	if (pipe(child_handler_pipe) < 0)
-		die ("Could not set up pipe for child handler");
-
-	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
+	pfd = xcalloc(socknum, sizeof(struct pollfd));
 
 	for (i = 0; i < socknum; i++) {
 		pfd[i].fd = socklist[i];
 		pfd[i].events = POLLIN;
 	}
-	pfd[socknum].fd = child_handler_pipe[0];
-	pfd[socknum].events = POLLIN;
-
-	child_handler(0);
 
 	for (;;) {
 		int i;
+		int olderrno;
+
+		i = poll(pfd, socknum, -1);
+		olderrno = errno;
 
-		if (poll(pfd, socknum + 1, -1) < 0) {
-			if (errno != EINTR) {
-				error("poll failed, resuming: %s",
-				      strerror(errno));
+		check_dead_children();
+
+		if (i < 0) {
+			if (olderrno != EINTR) {
+				logerror("poll failed, resuming: %s",
+				      strerror(olderrno));
 				sleep(1);
 			}
 			continue;
 		}
-		if (pfd[socknum].revents & POLLIN) {
-			read(child_handler_pipe[0], &i, 1);
-			check_dead_children();
-		}
 
 		for (i = 0; i < socknum; i++) {
 			if (pfd[i].revents & POLLIN) {
@@ -1055,10 +978,7 @@ int main(int argc, char **argv)
 	gid_t gid = 0;
 	int i;
 
-	/* Without this we cannot rely on waitpid() to tell
-	 * what happened to our children.
-	 */
-	signal(SIGCHLD, SIG_DFL);
+	child_handler(0);
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
@@ -1105,6 +1025,10 @@ int main(int argc, char **argv)
 			init_timeout = atoi(arg+15);
 			continue;
 		}
+		if (!prefixcmp(arg, "--max-connections=")) {
+			max_connections = atoi(arg+18);
+			continue;
+		}
 		if (!strcmp(arg, "--strict-paths")) {
 			strict_paths = 1;
 			continue;
@@ -1178,9 +1102,11 @@ int main(int argc, char **argv)
 	}
 
 	if (log_syslog) {
-		openlog("git-daemon", 0, LOG_DAEMON);
+		openlog("git-daemon", LOG_PID, LOG_DAEMON);
 		set_die_routine(daemon_die);
 	}
+	else			    /* so that logging into a file is atomic */
+		setlinebuf(stderr);
 
 	if (inetd_mode && (group_name || user_name))
 		die("--user and --group are incompatible with --inetd");
@@ -1233,8 +1159,10 @@ int main(int argc, char **argv)
 		return execute(peer);
 	}
 
-	if (detach)
+	if (detach) {
 		daemonize();
+		loginfo("Ready to rumble");
+	}
 	else
 		sanitize_stdfds();
 

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Marcus Griep]

I'd still suggest this get split up.  Even though the patches may
be dependent upon one another, each logical unit should be it's
own patch and commit in the Git repository.  Submit them as a
patch series with a summary and it'll make each part clear as
to what was modified to achieve what, and easier to review.

Also, the title to your patch is malformed.  Perhaps your MUA doing
some unintended wrapping?

Marcus

Stephen R. van den Berg wrote:
> making the signal handler almost a no-op.
> Fix the killing code to actually be smart instead of the
> pseudo-random mess.
> Get rid of the silly fixed array of children and make
> max-connections dynamic and configurable in the process.
> Make git-daemon a proper syslogging citizen with PID-info.
> Simplify the overzealous double buffering in the logroutine,
> remove the artificial maximum logline length in the process.
> Changed two calls to error() into logerror().
> 
> Signed-off-by: Stephen R. van den Berg <srb@cuci.nl>
> ---
> 
> Took out the vsyslog().
> Cleanup a bit left and right.
> Can this go in in one piece?  Or do I need to split it up?
> Like I said, large parts of the split-up are obsolete due do getting
> deleted.
> 
>  Documentation/git-daemon.txt |    8 +
>  daemon.c                     |  285 ++++++++++++++++--------------------------
>  2 files changed, 114 insertions(+), 179 deletions(-)
> 
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index 4ba4b75..48bcc25 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -9,8 +9,9 @@ SYNOPSIS
>  --------
>  [verse]
>  'git daemon' [--verbose] [--syslog] [--export-all]
> -	     [--timeout=n] [--init-timeout=n] [--strict-paths]
> -	     [--base-path=path] [--user-path | --user-path=path]
> +	     [--timeout=n] [--init-timeout=n] [--max-connections=n]
> +	     [--strict-paths] [--base-path=path] [--base-path-relaxed]
> +	     [--user-path | --user-path=path]
>  	     [--interpolated-path=pathtemplate]
>  	     [--reuseaddr] [--detach] [--pid-file=file]
>  	     [--enable=service] [--disable=service]
> @@ -99,6 +100,9 @@ OPTIONS
>  	it takes for the server to process the sub-request and time spent
>  	waiting for next client's request.
>  
> +--max-connections::
> +	Maximum number of concurrent clients, defaults to 32.
> +
>  --syslog::
>  	Log to syslog instead of stderr. Note that this option does not imply
>  	--verbose, thus by default only error conditions will be logged.
> diff --git a/daemon.c b/daemon.c
> index 1c00305..82eb224 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -16,12 +16,11 @@
>  static int log_syslog;
>  static int verbose;
>  static int reuseaddr;
> -static int child_handler_pipe[2];
>  
>  static const char daemon_usage[] =
>  "git daemon [--verbose] [--syslog] [--export-all]\n"
> -"           [--timeout=n] [--init-timeout=n] [--strict-paths]\n"
> -"           [--base-path=path] [--base-path-relaxed]\n"
> +"           [--timeout=n] [--init-timeout=n] [--max-connections=n]\n"
> +"           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
>  "           [--user-path | --user-path=path]\n"
>  "           [--interpolated-path=path]\n"
>  "           [--reuseaddr] [--detach] [--pid-file=file]\n"
> @@ -78,38 +77,19 @@ static struct interp interp_table[] = {
>  
>  static void logreport(int priority, const char *err, va_list params)
>  {
> -	/* We should do a single write so that it is atomic and output
> -	 * of several processes do not get intermingled. */
> -	char buf[1024];
> -	int buflen;
> -	int maxlen, msglen;
> -
> -	/* sizeof(buf) should be big enough for "[pid] \n" */
> -	buflen = snprintf(buf, sizeof(buf), "[%ld] ", (long) getpid());
> -
> -	maxlen = sizeof(buf) - buflen - 1; /* -1 for our own LF */
> -	msglen = vsnprintf(buf + buflen, maxlen, err, params);
> -
>  	if (log_syslog) {
> +		char buf[1024];
> +		vsnprintf(buf, sizeof(buf), err, params);
>  		syslog(priority, "%s", buf);
> -		return;
>  	}
> -
> -	/* maxlen counted our own LF but also counts space given to
> -	 * vsnprintf for the terminating NUL.  We want to make sure that
> -	 * we have space for our own LF and NUL after the "meat" of the
> -	 * message, so truncate it at maxlen - 1.
> -	 */
> -	if (msglen > maxlen - 1)
> -		msglen = maxlen - 1;
> -	else if (msglen < 0)
> -		msglen = 0; /* Protect against weird return values. */
> -	buflen += msglen;
> -
> -	buf[buflen++] = '\n';
> -	buf[buflen] = '\0';
> -
> -	write_in_full(2, buf, buflen);
> +	else {
> +		/* Since stderr is set to linebuffered mode, the
> +		 * logging of different processes will not overlap
> +		 */
> +		fprintf(stderr, "[%d] ", (int)getpid());
> +		vfprintf(stderr, err, params);
> +		fputc('\n', stderr);
> +	}
>  }
>  
>  static void logerror(const char *err, ...)
> @@ -604,40 +584,38 @@ static int execute(struct sockaddr *addr)
>  	return -1;
>  }
>  
> +static int max_connections = 32;
>  
> -/*
> - * We count spawned/reaped separately, just to avoid any
> - * races when updating them from signals. The SIGCHLD handler
> - * will only update children_reaped, and the fork logic will
> - * only update children_spawned.
> - *
> - * MAX_CHILDREN should be a power-of-two to make the modulus
> - * operation cheap. It should also be at least twice
> - * the maximum number of connections we will ever allow.
> - */
> -#define MAX_CHILDREN 128
> -
> -static int max_connections = 25;
> -
> -/* These are updated by the signal handler */
> -static volatile unsigned int children_reaped;
> -static pid_t dead_child[MAX_CHILDREN];
> -
> -/* These are updated by the main loop */
> -static unsigned int children_spawned;
> -static unsigned int children_deleted;
> +static unsigned int live_children;
>  
>  static struct child {
> +	struct child*next;
>  	pid_t pid;
> -	int addrlen;
>  	struct sockaddr_storage address;
> -} live_child[MAX_CHILDREN];
> +} *firstborn;
>  
> -static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
> +static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)
>  {
> -	live_child[idx].pid = pid;
> -	live_child[idx].addrlen = addrlen;
> -	memcpy(&live_child[idx].address, addr, addrlen);
> +	struct child*newborn;
> +	newborn = xmalloc(sizeof *newborn);
> +	if (newborn) {
> +		struct child **cradle, *blanket;
> +
> +		live_children++;
> +		newborn->pid = pid;
> +		memcpy(memset(&newborn->address, 0, sizeof newborn->address),
> +		 addr, addrlen);
> +		for (cradle = &firstborn;
> +		     (blanket = *cradle);
> +		     cradle = &blanket->next)
> +			if (!memcmp(&blanket->address, &newborn->address,
> +				   sizeof newborn->address))
> +				break;
> +		newborn->next = blanket;
> +		*cradle = newborn;
> +	}
> +	else
> +		logerror("Out of memory spawning new child");
>  }
>  
>  /*
> @@ -646,107 +624,74 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)
>   * We move everything up by one, since the new "deleted" will
>   * be one higher.
>   */
> -static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)
> +static void remove_child(pid_t pid)
>  {
> -	struct child n;
> -
> -	deleted %= MAX_CHILDREN;
> -	spawned %= MAX_CHILDREN;
> -	if (live_child[deleted].pid == pid) {
> -		live_child[deleted].pid = -1;
> -		return;
> -	}
> -	n = live_child[deleted];
> -	for (;;) {
> -		struct child m;
> -		deleted = (deleted + 1) % MAX_CHILDREN;
> -		if (deleted == spawned)
> -			die("could not find dead child %d\n", pid);
> -		m = live_child[deleted];
> -		live_child[deleted] = n;
> -		if (m.pid == pid)
> -			return;
> -		n = m;
> -	}
> +	struct child **cradle, *blanket;
> +
> +	for (cradle = &firstborn;
> +	     (blanket = *cradle);
> +	     cradle = &blanket->next)
> +		if (blanket->pid == pid) {
> +			*cradle = blanket->next;
> +			live_children--;
> +			free(blanket);
> +			break;
> +		}
>  }
>  
>  /*
>   * This gets called if the number of connections grows
>   * past "max_connections".
>   *
> - * We _should_ start off by searching for connections
> - * from the same IP, and if there is some address wth
> - * multiple connections, we should kill that first.
> - *
> - * As it is, we just "randomly" kill 25% of the connections,
> - * and our pseudo-random generator sucks too. I have no
> - * shame.
> - *
> - * Really, this is just a place-holder for a _real_ algorithm.
> + * We kill the newest connection from a duplicate IP first.
> + * If there are no duplicates, then the newest connection is killed,
> + * in order to allow long running clones to actually complete.
>   */
> -static void kill_some_children(int signo, unsigned start, unsigned stop)
> +static void kill_some_child(int signo)
>  {
> -	start %= MAX_CHILDREN;
> -	stop %= MAX_CHILDREN;
> -	while (start != stop) {
> -		if (!(start & 3))
> -			kill(live_child[start].pid, signo);
> -		start = (start + 1) % MAX_CHILDREN;
> +	const struct child *blanket;
> +
> +	if ((blanket = firstborn)) {
> +		const struct child *next;
> +
> +		for (; (next = blanket->next); blanket = next)
> +			if (!memcmp(&blanket->address, &next->address,
> +				   sizeof next->address))
> +				break;
> +
> +		kill(blanket->pid, signo);
>  	}
>  }
>  
>  static void check_dead_children(void)
>  {
> -	unsigned spawned, reaped, deleted;
> -
> -	spawned = children_spawned;
> -	reaped = children_reaped;
> -	deleted = children_deleted;
> -
> -	while (deleted < reaped) {
> -		pid_t pid = dead_child[deleted % MAX_CHILDREN];
> -		const char *dead = pid < 0 ? " (with error)" : "";
> -
> -		if (pid < 0)
> -			pid = -pid;
> +	int status;
> +	pid_t pid;
>  
> -		/* XXX: Custom logging, since we don't wanna getpid() */
> -		if (verbose) {
> -			if (log_syslog)
> -				syslog(LOG_INFO, "[%d] Disconnected%s",
> -						pid, dead);
> -			else
> -				fprintf(stderr, "[%d] Disconnected%s\n",
> -						pid, dead);
> -		}
> -		remove_child(pid, deleted, spawned);
> -		deleted++;
> +	while ((pid = waitpid(-1, &status, WNOHANG))>0) {
> +		const char *dead = "";
> +		remove_child(pid);
> +		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
> +			dead = " (with error)";
> +		loginfo("[%d] Disconnected%s", (int)pid, dead);
>  	}
> -	children_deleted = deleted;
>  }
>  
>  static void check_max_connections(void)
>  {
>  	for (;;) {
> -		int active;
> -		unsigned spawned, deleted;
> -
>  		check_dead_children();
> -
> -		spawned = children_spawned;
> -		deleted = children_deleted;
> -
> -		active = spawned - deleted;
> -		if (active <= max_connections)
> +		if (live_children <= max_connections)
>  			break;
>  
>  		/* Kill some unstarted connections with SIGTERM */
> -		kill_some_children(SIGTERM, deleted, spawned);
> -		if (active <= max_connections << 1)
> +		kill_some_child(SIGTERM);
> +		check_dead_children();
> +		if (live_children <= max_connections << 1)
>  			break;
>  
>  		/* If the SIGTERM thing isn't helping use SIGKILL */
> -		kill_some_children(SIGKILL, deleted, spawned);
> +		kill_some_child(SIGKILL);
>  		sleep(1);
>  	}
>  }
> @@ -756,16 +701,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
>  	pid_t pid = fork();
>  
>  	if (pid) {
> -		unsigned idx;
> -
>  		close(incoming);
> -		if (pid < 0)
> +		if (pid < 0) {
> +			logerror("Couldn't fork %s", strerror(errno));
>  			return;
> +		}
>  
> -		idx = children_spawned % MAX_CHILDREN;
> -		children_spawned++;
> -		add_child(idx, pid, addr, addrlen);
> -
> +		add_child(pid, addr, addrlen);
>  		check_max_connections();
>  		return;
>  	}
> @@ -779,21 +721,10 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)
>  
>  static void child_handler(int signo)
>  {
> -	for (;;) {
> -		int status;
> -		pid_t pid = waitpid(-1, &status, WNOHANG);
> -
> -		if (pid > 0) {
> -			unsigned reaped = children_reaped;
> -			if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
> -				pid = -pid;
> -			dead_child[reaped % MAX_CHILDREN] = pid;
> -			children_reaped = reaped + 1;
> -			write(child_handler_pipe[1], &status, 1);
> -			continue;
> -		}
> -		break;
> -	}
> +	/* Otherwise empty handler because systemcalls will get interrupted
> +	 * upon signal receipt
> +	 * SysV needs the handler to be reinstated
> +	 */
>  	signal(SIGCHLD, child_handler);
>  }
>  
> @@ -836,7 +767,7 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)
>  		if (sockfd < 0)
>  			continue;
>  		if (sockfd >= FD_SETSIZE) {
> -			error("too large socket descriptor.");
> +			logerror("too large socket descriptor.");
>  			close(sockfd);
>  			continue;
>  		}
> @@ -936,35 +867,30 @@ static int service_loop(int socknum, int *socklist)
>  	struct pollfd *pfd;
>  	int i;
>  
> -	if (pipe(child_handler_pipe) < 0)
> -		die ("Could not set up pipe for child handler");
> -
> -	pfd = xcalloc(socknum + 1, sizeof(struct pollfd));
> +	pfd = xcalloc(socknum, sizeof(struct pollfd));
>  
>  	for (i = 0; i < socknum; i++) {
>  		pfd[i].fd = socklist[i];
>  		pfd[i].events = POLLIN;
>  	}
> -	pfd[socknum].fd = child_handler_pipe[0];
> -	pfd[socknum].events = POLLIN;
> -
> -	child_handler(0);
>  
>  	for (;;) {
>  		int i;
> +		int olderrno;
> +
> +		i = poll(pfd, socknum, -1);
> +		olderrno = errno;
>  
> -		if (poll(pfd, socknum + 1, -1) < 0) {
> -			if (errno != EINTR) {
> -				error("poll failed, resuming: %s",
> -				      strerror(errno));
> +		check_dead_children();
> +
> +		if (i < 0) {
> +			if (olderrno != EINTR) {
> +				logerror("poll failed, resuming: %s",
> +				      strerror(olderrno));
>  				sleep(1);
>  			}
>  			continue;
>  		}
> -		if (pfd[socknum].revents & POLLIN) {
> -			read(child_handler_pipe[0], &i, 1);
> -			check_dead_children();
> -		}
>  
>  		for (i = 0; i < socknum; i++) {
>  			if (pfd[i].revents & POLLIN) {
> @@ -1055,10 +981,7 @@ int main(int argc, char **argv)
>  	gid_t gid = 0;
>  	int i;
>  
> -	/* Without this we cannot rely on waitpid() to tell
> -	 * what happened to our children.
> -	 */
> -	signal(SIGCHLD, SIG_DFL);
> +	child_handler(0);
>  
>  	for (i = 1; i < argc; i++) {
>  		char *arg = argv[i];
> @@ -1105,6 +1028,10 @@ int main(int argc, char **argv)
>  			init_timeout = atoi(arg+15);
>  			continue;
>  		}
> +		if (!prefixcmp(arg, "--max-connections=")) {
> +			max_connections = atoi(arg+18);
> +			continue;
> +		}
>  		if (!strcmp(arg, "--strict-paths")) {
>  			strict_paths = 1;
>  			continue;
> @@ -1178,9 +1105,11 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (log_syslog) {
> -		openlog("git-daemon", 0, LOG_DAEMON);
> +		openlog("git-daemon", LOG_PID, LOG_DAEMON);
>  		set_die_routine(daemon_die);
>  	}
> +	else			    /* so that logging into a file is atomic */
> +		setlinebuf(stderr);
>  
>  	if (inetd_mode && (group_name || user_name))
>  		die("--user and --group are incompatible with --inetd");
> @@ -1233,8 +1162,10 @@ int main(int argc, char **argv)
>  		return execute(peer);
>  	}
>  
> -	if (detach)
> +	if (detach) {
>  		daemonize();
> +		loginfo("Ready to rumble");
> +	}
>  	else
>  		sanitize_stdfds();
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Marcus Griep
GPG Key ID: 0x5E968152
——
http://www.boohaunt.net
את.ψο´

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Junio C Hamano]

"Stephen R. van den Berg" <srb@cuci.nl> writes:

> Separating it causes two things:
> a. The patches to become dependent on each other in the timeline.
> b. More (redundant) work, because some parts that need to be rewritten, get
>    deleted by the following patch(es).

These are actually desirable properties from reviewability point of view.

>> - I see you have a call to vsyslog, which is the first user of the
>>   function.  How portable is it (the patch coming from you, I know
>>   Solaris would have it, and recent 4BSD also would, but what about the
>>   others)?
>
> Cygwin has it, Solaris does, Linux does, MacOSX does.
> AIX and HPUX don't, perhaps.
> I'll see what I can do to avoid it, yet simplify the code.

That's one of the reasons why I asked you to split it to three patches, so
that the syslog change can potentially be independently replaced with a
better alternative.

In any case, it is already late in the rc cycle; I'd like to apply your
earlier "In SysV, signal(SIGCHLD) need to be rearmed" patch and nothing
else for now.  The clean-up is very attractive but can be done post 1.6.0.

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

Junio C Hamano wrote:
>"Stephen R. van den Berg" <srb@cuci.nl> writes:

>> Separating it causes two things:
>> a. The patches to become dependent on each other in the timeline.
>> b. More (redundant) work, because some parts that need to be rewritten, get
>>    deleted by the following patch(es).

>These are actually desirable properties from reviewability point of view.

Ok, I'll look into the splitup.

>>> - I see you have a call to vsyslog, which is the first user of the
>>>   function.  How portable is it (the patch coming from you, I know
>>>   Solaris would have it, and recent 4BSD also would, but what about the
>>>   others)?

>> Cygwin has it, Solaris does, Linux does, MacOSX does.
>> AIX and HPUX don't, perhaps.
>> I'll see what I can do to avoid it, yet simplify the code.

>That's one of the reasons why I asked you to split it to three patches, so
>that the syslog change can potentially be independently replaced with a
>better alternative.

Well, I've already found an alternative, and looks a lot more appealing
than the buffer-juggling code of before.

>In any case, it is already late in the rc cycle; I'd like to apply your
>earlier "In SysV, signal(SIGCHLD) need to be rearmed" patch and nothing
>else for now.  The clean-up is very attractive but can be done post 1.6.0.

No problem.  That's the reason I chipped that first patch off; it's a
direct bugfix.  And BTW, don't mistake me for a "Solaris guy".  I've
been using Linux almost exclusively since 1991.  My SunOS/Solaris knowledge
has been fading steadily ever since.  It's just that I had to install
git-daemon on someone else's Solaris box just recently.
-- 
Sincerely,
           Stephen R. van den Berg.

"And now for something *completely* different!"

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

Stephen R. van den Berg wrote:
>Junio C Hamano wrote:
>> - Taking advantage of poll() getting interrupted by SIGCHLD, so that you
>>   do not have to do anything in the signal handler, is so obvious that I
>>   am actually ashamed of not having to think of it the last time we
>>   touched this code.  Is there a poll() that does not return EINTR but
>>   just call the handler and restart after that as if nothing has
>>   happened, I have to wonder...

>Only if the signal is set to SIG_IGN on all systems I worked with since
>1987.

And even if it doesn't get interrupted and restarts, it means that it
merely leaves some zombies uncollected until the next new connection to
the daemon; nothing to get worried about.
-- 
Sincerely,
           Stephen R. van den Berg.

"And now for something *completely* different!"

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Junio C Hamano]

"Stephen R. van den Berg" <srb@cuci.nl> writes:

> Junio C Hamano wrote:
>>"Stephen R. van den Berg" <srb@cuci.nl> writes:
>>Sorry, but this does too many things in one patch.
>
> Yes, I know, got carried away.  Then again, the code has a lot of
> overlapping places (spacewise); I kind of leapt from one place to the
> next; you fix one thing, and then the next wart stares you in the face.
> I'll see if I can split it up, if that suits you better.
>
>> - Taking advantage of poll() getting interrupted by SIGCHLD, so that you
>>   do not have to do anything in the signal handler, is so obvious that I
>>   am actually ashamed of not having to think of it the last time we
>>   touched this code.  Is there a poll() that does not return EINTR but
>>   just call the handler and restart after that as if nothing has
>>   happened, I have to wonder...
>
> Only if the signal is set to SIG_IGN on all systems I worked with since
> 1987.

Yeah, rub it in.  That's why I said I am actually ashamed that I did not
notice that as an obviously much more simpler approach.

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Alex Riesen]

Stephen R. van den Berg, Tue, Aug 12, 2008 23:25:35 +0200:
> +        struct child*newborn;

You may want to reformat the patch using tabs instead of spaces.

> +        newborn = xmalloc(sizeof *newborn);

The custom here is to use "sizeof(type)". Brackets and typename.

> +        if (newborn) {
> +	        struct child**cradle,*blanket;

"struct child **cradle, *blanket;" (the spaces before asterisks)

> +		memcpy(memset(&newborn->address, 0, sizeof newborn->address),
> +		 addr, addrlen);

Aren't separate calls easier to read (and type)?

	memset(&newborn->address, 0, sizeof(newborn->address));
	memcpy(&newborn->address, addr, addrlen);

> -static void kill_some_children(int signo, unsigned start, unsigned stop)
> +static void kill_some_child(int signo)
>  {
> -	start %= MAX_CHILDREN;
> -	stop %= MAX_CHILDREN;
> -	while (start != stop) {
> -		if (!(start & 3))
> -			kill(live_child[start].pid, signo);
> -		start = (start + 1) % MAX_CHILDREN;
> +	const struct child *blanket;
> +
> +	if ((blanket = firstborn)) {

	if (firstborn) {
	    const struct child *blanket = firstborn;

You don't even use blanket outside of the "if".

>  static void check_dead_children(void)
>  {
> +		loginfo("[%d] Disconnected%s", (int)pid, dead);

BTW, why do you need that pid_t->int cast?

> @@ -1105,6 +1026,10 @@ int main(int argc, char **argv)
>  			init_timeout = atoi(arg+15);
>  			continue;
>  		}
> +		if (!prefixcmp(arg, "--max-connections=")) {
> +			max_connections = atoi(arg+18);

An error checking wouldn't go amiss. And it can't be done with atoi
(consider strtol).

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

Alex Riesen wrote:
>Stephen R. van den Berg, Tue, Aug 12, 2008 23:25:35 +0200:
>> +        struct child*newborn;

>You may want to reformat the patch using tabs instead of spaces.

Tried to do it right, but apparently missed a few spots, thanks.

>> +        newborn = xmalloc(sizeof *newborn);

>The custom here is to use "sizeof(type)". Brackets and typename.

Well, as for the parens, current git source shows a ratio of 26 : 887
in your favour; apparently not everyone agrees, but most do.
As for the typename, that rather uncommon, in most cases actual
variables are used (with good reason).

>> +        if (newborn) {
>> +	        struct child**cradle,*blanket;

>"struct child **cradle, *blanket;" (the spaces before asterisks)

I'll be more careful.

>> +		memcpy(memset(&newborn->address, 0, sizeof newborn->address),
>> +		 addr, addrlen);

>Aren't separate calls easier to read (and type)?

Possibly, yes.

>	memset(&newborn->address, 0, sizeof(newborn->address));
>	memcpy(&newborn->address, addr, addrlen);

But it results in more machinecode in most cases.  Sorry, is my builtin
micro-optimiser at work.

>> -static void kill_some_children(int signo, unsigned start, unsigned stop)
>> +static void kill_some_child(int signo)
>>  {
>> -	start %= MAX_CHILDREN;
>> -	stop %= MAX_CHILDREN;
>> -	while (start != stop) {
>> -		if (!(start & 3))
>> -			kill(live_child[start].pid, signo);
>> -		start = (start + 1) % MAX_CHILDREN;
>> +	const struct child *blanket;
>> +
>> +	if ((blanket = firstborn)) {

>	if (firstborn) {
>	    const struct child *blanket = firstborn;

>You don't even use blanket outside of the "if".

Good thinking.  I restructured the code a few times, previously this
wasn't possible.

>>  static void check_dead_children(void)
>>  {
>> +		loginfo("[%d] Disconnected%s", (int)pid, dead);

>BTW, why do you need that pid_t->int cast?

Because pid_t might be wider than int (unlikely, but possible),
and then this call might crash the program.

>> @@ -1105,6 +1026,10 @@ int main(int argc, char **argv)
>>  			init_timeout = atoi(arg+15);
>>  			continue;

>> +		if (!prefixcmp(arg, "--max-connections=")) {
>> +			max_connections = atoi(arg+18);

>An error checking wouldn't go amiss. And it can't be done with atoi
>(consider strtol).

I merely copied the other argument parsing methods, didn't want to
improve this, just functionally equivalent (will do fine here, IMO).
-- 
Sincerely,
           Stephen R. van den Berg.

Father's Day Special at the local clinic -- Vasectomy!

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Alex Riesen]

Stephen R. van den Berg, Wed, Aug 13, 2008 01:12:10 +0200:
> Alex Riesen wrote:
> >Stephen R. van den Berg, Tue, Aug 12, 2008 23:25:35 +0200:
> >> @@ -1105,6 +1026,10 @@ int main(int argc, char **argv)
> >>  			init_timeout = atoi(arg+15);
> >>  			continue;
> 
> >> +		if (!prefixcmp(arg, "--max-connections=")) {
> >> +			max_connections = atoi(arg+18);
> 
> >An error checking wouldn't go amiss. And it can't be done with atoi
> >(consider strtol).
> 
> I merely copied the other argument parsing methods, didn't want to
> improve this, just functionally equivalent (will do fine here, IMO).

In case of error max_connections will be 0. How does your code handle
it? It is a server side program, which supposed to provide reliable
service. In this case, an operator mistake is not even noticable until
the first request and even than the clients have to complain first.

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Stephen R. van den Berg]

Alex Riesen wrote:
>Stephen R. van den Berg, Wed, Aug 13, 2008 01:12:10 +0200:
>> Alex Riesen wrote:
>> I merely copied the other argument parsing methods, didn't want to
>> improve this, just functionally equivalent (will do fine here, IMO).

>In case of error max_connections will be 0. How does your code handle
>it? It is a server side program, which supposed to provide reliable
>service. In this case, an operator mistake is not even noticable until
>the first request and even than the clients have to complain first.

Well, same thing that goes wrong when the timeout is set too low,
of course.
But, you have a point, I'll provide a reasonable cap and support
unlimited as well.
-- 
Sincerely,
           Stephen R. van den Berg.

"And now for something *completely* different!"

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Andreas Ericsson]

Stephen R. van den Berg wrote:
> +		/* Since stderr is set to linebuffered mode, the
> +		 * logging of different processes will not overlap
> +		 */
> +		fprintf(stderr, "[%d] ", (int)getpid());
> +		vfprintf(stderr, err, params);
> +		fputc('\n', stderr);

stderr is unbuffered. It's stdout that is linebuffered.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] git-daemon: Simplify child management and associated logging by

[Alex Riesen]

2008/8/13 Andreas Ericsson <ae@op5.se>:
> Stephen R. van den Berg wrote:
>>
>> +               /* Since stderr is set to linebuffered mode, the
>> +                * logging of different processes will not overlap
>> +                */
>> +               fprintf(stderr, "[%d] ", (int)getpid());
>> +               vfprintf(stderr, err, params);
>> +               fputc('\n', stderr);
>
> stderr is unbuffered. It's stdout that is linebuffered.
>

Nah, he linebuffered stderr explicitely before.