{"thread":{"id":"14976","subject":"[PATCH 3/3] git-daemon: rewrite kindergarden","startedAt":"2008-08-13T08:43:31Z","lastAt":"2008-08-14T13:41:35Z","messageCount":16,"participants":["Stephen R. van den Berg","Petr Baudis","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"87026","messageId":"20080813084331.30845.21129.stgit@aristoteles.cuci.nl","threadId":"14976","inReplyTo":"20080813084330.30845.89753.stgit@aristoteles.cuci.nl","subject":"[PATCH 3/3] git-daemon: rewrite kindergarden","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T08:43:31Z","receivedAt":"2008-08-13T08:43:31Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Get rid of the silly fixed array of children and make\nmax-connections dynamic and configurable in the process.\nFix the killing code to actually be smart instead of the\npseudo-random mess.\nAvoid forking if too busy already.\n\nSigned-off-by: Stephen R. van den Berg <srb@cuci.nl>\n---\n\n Documentation/git-daemon.txt |    9 +-\n daemon.c                     |  215 +++++++++++++++---------------------------\n 2 files changed, 84 insertions(+), 140 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 4ba4b75..b08a08c 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -9,8 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git daemon' [--verbose] [--syslog] [--export-all]\n-\t     [--timeout=n] [--init-timeout=n] [--strict-paths]\n-\t     [--base-path=path] [--user-path | --user-path=path]\n+\t     [--timeout=n] [--init-timeout=n] [--max-connections=n]\n+\t     [--strict-paths] [--base-path=path] [--base-path-relaxed]\n+\t     [--user-path | --user-path=path]\n \t     [--interpolated-path=pathtemplate]\n \t     [--reuseaddr] [--detach] [--pid-file=file]\n \t     [--enable=service] [--disable=service]\n@@ -99,6 +100,10 @@ OPTIONS\n \tit takes for the server to process the sub-request and time spent\n \twaiting for next client's request.\n \n+--max-connections::\n+\tMaximum number of concurrent clients, defaults to 32.  Set it to\n+\tzero for no limit.\n+\n --syslog::\n \tLog to syslog instead of stderr. Note that this option does not imply\n \t--verbose, thus by default only error conditions will be logged.\ndiff --git a/daemon.c b/daemon.c\nindex 19d620c..a7a735c 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -19,8 +19,8 @@ static int reuseaddr;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n-\"           [--timeout=n] [--init-timeout=n] [--strict-paths]\\n\"\n-\"           [--base-path=path] [--base-path-relaxed]\\n\"\n+\"           [--timeout=n] [--init-timeout=n] [--max-connections=n]\\n\"\n+\"           [--strict-paths] [--base-path=path] [--base-path-relaxed]\\n\"\n \"           [--user-path | --user-path=path]\\n\"\n \"           [--interpolated-path=path]\\n\"\n \"           [--reuseaddr] [--detach] [--pid-file=file]\\n\"\n@@ -584,40 +584,37 @@ static int execute(struct sockaddr *addr)\n \treturn -1;\n }\n \n+static int max_connections = 32;\n \n-/*\n- * We count spawned/reaped separately, just to avoid any\n- * races when updating them from signals. The SIGCHLD handler\n- * will only update children_reaped, and the fork logic will\n- * only update children_spawned.\n- *\n- * MAX_CHILDREN should be a power-of-two to make the modulus\n- * operation cheap. It should also be at least twice\n- * the maximum number of connections we will ever allow.\n- */\n-#define MAX_CHILDREN 128\n-\n-static int max_connections = 25;\n-\n-/* These are updated by the signal handler */\n-static volatile unsigned int children_reaped;\n-static pid_t dead_child[MAX_CHILDREN];\n-\n-/* These are updated by the main loop */\n-static unsigned int children_spawned;\n-static unsigned int children_deleted;\n+static unsigned int live_children;\n \n static struct child {\n+\tstruct child*next;\n \tpid_t pid;\n-\tint addrlen;\n \tstruct sockaddr_storage address;\n-} live_child[MAX_CHILDREN];\n+} *firstborn;\n \n-static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n+static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n {\n-\tlive_child[idx].pid = pid;\n-\tlive_child[idx].addrlen = addrlen;\n-\tmemcpy(&live_child[idx].address, addr, addrlen);\n+\tstruct child*newborn;\n+\tnewborn = xcalloc(1, sizeof *newborn);\n+\tif (newborn) {\n+\t\tstruct child **cradle, *blanket;\n+\n+\t\tlive_children++;\n+\t\tnewborn->pid = pid;\n+\t\tmemcpy(&newborn->address, addr, addrlen);\n+\t\tfor (cradle = &firstborn;\n+\t\t     (blanket = *cradle);\n+\t\t     cradle = &blanket->next)\n+\t\t\tif (!memcmp(&blanket->address, &newborn->address,\n+\t\t\t\t   sizeof newborn->address))\n+\t\t\t\tbreak;\n+\t\tnewborn->next = blanket;\n+\t\t*cradle = newborn;\n+\t}\n+\telse\n+\t\tlogerror(\"Out of memory spawning new child\");\n }\n \n /*\n@@ -626,142 +623,78 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n  * We move everything up by one, since the new \"deleted\" will\n  * be one higher.\n  */\n-static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)\n+static void remove_child(pid_t pid)\n {\n-\tstruct child n;\n+\tstruct child **cradle, *blanket;\n \n-\tdeleted %= MAX_CHILDREN;\n-\tspawned %= MAX_CHILDREN;\n-\tif (live_child[deleted].pid == pid) {\n-\t\tlive_child[deleted].pid = -1;\n-\t\treturn;\n-\t}\n-\tn = live_child[deleted];\n-\tfor (;;) {\n-\t\tstruct child m;\n-\t\tdeleted = (deleted + 1) % MAX_CHILDREN;\n-\t\tif (deleted == spawned)\n-\t\t\tdie(\"could not find dead child %d\\n\", pid);\n-\t\tm = live_child[deleted];\n-\t\tlive_child[deleted] = n;\n-\t\tif (m.pid == pid)\n-\t\t\treturn;\n-\t\tn = m;\n-\t}\n+\tfor (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)\n+\t\tif (blanket->pid == pid) {\n+\t\t\t*cradle = blanket->next;\n+\t\t\tlive_children--;\n+\t\t\tfree(blanket);\n+\t\t\tbreak;\n+\t\t}\n }\n \n /*\n  * This gets called if the number of connections grows\n  * past \"max_connections\".\n  *\n- * We _should_ start off by searching for connections\n- * from the same IP, and if there is some address wth\n- * multiple connections, we should kill that first.\n- *\n- * As it is, we just \"randomly\" kill 25% of the connections,\n- * and our pseudo-random generator sucks too. I have no\n- * shame.\n- *\n- * Really, this is just a place-holder for a _real_ algorithm.\n+ * We kill the newest connection from a duplicate IP.\n  */\n-static void kill_some_children(int signo, unsigned start, unsigned stop)\n+static void kill_some_child()\n {\n-\tstart %= MAX_CHILDREN;\n-\tstop %= MAX_CHILDREN;\n-\twhile (start != stop) {\n-\t\tif (!(start & 3))\n-\t\t\tkill(live_child[start].pid, signo);\n-\t\tstart = (start + 1) % MAX_CHILDREN;\n-\t}\n-}\n-\n-static void check_dead_children(void)\n-{\n-\tunsigned spawned, reaped, deleted;\n-\n-\tfor (;;) {\n-\t    int status;\n-\t    pid_t pid = waitpid(-1, &status, WNOHANG);\n-\t\n-\t    if (pid > 0) {\n-\t        unsigned reaped = children_reaped;\n-\t        if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n-\t            pid = -pid;\n-\t        dead_child[reaped % MAX_CHILDREN] = pid;\n-\t        children_reaped = reaped + 1;\n-\t        continue;\n-\t    }\n-\t    break;\n-\t}\n+\tconst struct child *blanket;\n \n-\tspawned = children_spawned;\n-\treaped = children_reaped;\n-\tdeleted = children_deleted;\n+\tif ((blanket = firstborn)) {\n+\t\tconst struct child *next;\n \n-\twhile (deleted < reaped) {\n-\t\tpid_t pid = dead_child[deleted % MAX_CHILDREN];\n-\t\tconst char *dead = pid < 0 ? \" (with error)\" : \"\";\n-\n-\t\tif (pid < 0)\n-\t\t\tpid = -pid;\n-\n-\t\t/* XXX: Custom logging, since we don't wanna getpid() */\n-\t\tif (verbose) {\n-\t\t\tif (log_syslog)\n-\t\t\t\tsyslog(LOG_INFO, \"[%d] Disconnected%s\",\n-\t\t\t\t\t\tpid, dead);\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"[%d] Disconnected%s\\n\",\n-\t\t\t\t\t\tpid, dead);\n-\t\t}\n-\t\tremove_child(pid, deleted, spawned);\n-\t\tdeleted++;\n+\t\tfor (; (next = blanket->next); blanket = next)\n+\t\t\tif (!memcmp(&blanket->address, &next->address,\n+\t\t\t\t   sizeof next->address)) {\n+\t\t\t\tkill(blanket->pid, SIGTERM);\n+\t\t\t\tbreak;\n+\t\t\t}\n \t}\n-\tchildren_deleted = deleted;\n }\n \n-static void check_max_connections(void)\n+static void check_dead_children(void)\n {\n-\tfor (;;) {\n-\t\tint active;\n-\t\tunsigned spawned, deleted;\n-\n-\t\tcheck_dead_children();\n-\n-\t\tspawned = children_spawned;\n-\t\tdeleted = children_deleted;\n-\n-\t\tactive = spawned - deleted;\n-\t\tif (active <= max_connections)\n-\t\t\tbreak;\n-\n-\t\t/* Kill some unstarted connections with SIGTERM */\n-\t\tkill_some_children(SIGTERM, deleted, spawned);\n-\t\tif (active <= max_connections << 1)\n-\t\t\tbreak;\n+\tint status;\n+\tpid_t pid;\n \n-\t\t/* If the SIGTERM thing isn't helping use SIGKILL */\n-\t\tkill_some_children(SIGKILL, deleted, spawned);\n-\t\tsleep(1);\n+\twhile ((pid = waitpid(-1, &status, WNOHANG))>0) {\n+\t\tconst char *dead = \"\";\n+\t\tremove_child(pid);\n+\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t\t\tdead = \" (with error)\";\n+\t\tloginfo(\"[%d] Disconnected%s\", (int)pid, dead);\n \t}\n }\n \n static void handle(int incoming, struct sockaddr *addr, int addrlen)\n {\n-\tpid_t pid = fork();\n+\tpid_t pid;\n \n-\tif (pid) {\n-\t\tunsigned idx;\n+\tif (max_connections && live_children >= max_connections) {\n+\t\tkill_some_child();\n+\t\tsleep(1);\t\t\t /* give it some time to die */\n+\t\tcheck_dead_children();\n+\t\tif (live_children >= max_connections) {\n+\t\t\tclose(incoming);\n+\t\t\tlogerror(\"Too many children, dropping connection\");\n+\t\t\treturn;\n+\t\t}\n+\t}\n \n+\tif ((pid = fork())) {\n \t\tclose(incoming);\n-\t\tif (pid < 0)\n+\t\tif (pid < 0) {\n+\t\t\tlogerror(\"Couldn't fork %s\", strerror(errno));\n \t\t\treturn;\n+\t\t}\n \n-\t\tidx = children_spawned % MAX_CHILDREN;\n-\t\tchildren_spawned++;\n-\t\tadd_child(idx, pid, addr, addrlen);\n-\n-\t\tcheck_max_connections();\n+\t\tadd_child(pid, addr, addrlen);\n \t\treturn;\n \t}\n \n@@ -1081,6 +1014,12 @@ int main(int argc, char **argv)\n \t\t\tinit_timeout = atoi(arg+15);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--max-connections=\")) {\n+\t\t\tmax_connections = atoi(arg+18);\n+\t\t\tif (max_connections<=0)\n+\t\t\t\tmax_connections=0;\t        /* unlimited */\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--strict-paths\")) {\n \t\t\tstrict_paths = 1;\n \t\t\tcontinue;\n"},{"id":"87027","messageId":"20080813084330.30845.89753.stgit@aristoteles.cuci.nl","threadId":"14976","inReplyTo":null,"subject":"[PATCH 1/3] git-daemon: logging done right","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T08:43:31Z","receivedAt":"2008-08-13T08:43:31Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Make git-daemon a proper syslogging citizen with PID-info.\nSimplify the overzealous double buffering in the logroutine.\nCall logerror() instead of error().\n\nSigned-off-by: Stephen R. van den Berg <srb@cuci.nl>\n---\n\n daemon.c |   49 +++++++++++++++++--------------------------------\n 1 files changed, 17 insertions(+), 32 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 1c00305..774b2ce 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -78,38 +78,19 @@ static struct interp interp_table[] = {\n \n static void logreport(int priority, const char *err, va_list params)\n {\n-\t/* We should do a single write so that it is atomic and output\n-\t * of several processes do not get intermingled. */\n-\tchar buf[1024];\n-\tint buflen;\n-\tint maxlen, msglen;\n-\n-\t/* sizeof(buf) should be big enough for \"[pid] \\n\" */\n-\tbuflen = snprintf(buf, sizeof(buf), \"[%ld] \", (long) getpid());\n-\n-\tmaxlen = sizeof(buf) - buflen - 1; /* -1 for our own LF */\n-\tmsglen = vsnprintf(buf + buflen, maxlen, err, params);\n-\n \tif (log_syslog) {\n+\t\tchar buf[1024];\n+\t\tvsnprintf(buf, sizeof(buf), err, params);\n \t\tsyslog(priority, \"%s\", buf);\n-\t\treturn;\n \t}\n-\n-\t/* maxlen counted our own LF but also counts space given to\n-\t * vsnprintf for the terminating NUL.  We want to make sure that\n-\t * we have space for our own LF and NUL after the \"meat\" of the\n-\t * message, so truncate it at maxlen - 1.\n-\t */\n-\tif (msglen > maxlen - 1)\n-\t\tmsglen = maxlen - 1;\n-\telse if (msglen < 0)\n-\t\tmsglen = 0; /* Protect against weird return values. */\n-\tbuflen += msglen;\n-\n-\tbuf[buflen++] = '\\n';\n-\tbuf[buflen] = '\\0';\n-\n-\twrite_in_full(2, buf, buflen);\n+\telse {\n+\t\t/* Since stderr is set to linebuffered mode, the\n+\t\t * logging of different processes will not overlap\n+\t\t */\n+\t\tfprintf(stderr, \"[%d] \", (int)getpid());\n+\t\tvfprintf(stderr, err, params);\n+\t\tfputc('\\n', stderr);\n+\t}\n }\n \n static void logerror(const char *err, ...)\n@@ -836,7 +817,7 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)\n \t\tif (sockfd < 0)\n \t\t\tcontinue;\n \t\tif (sockfd >= FD_SETSIZE) {\n-\t\t\terror(\"too large socket descriptor.\");\n+\t\t\tlogerror(\"too large socket descriptor.\");\n \t\t\tclose(sockfd);\n \t\t\tcontinue;\n \t\t}\n@@ -1178,9 +1159,11 @@ int main(int argc, char **argv)\n \t}\n \n \tif (log_syslog) {\n-\t\topenlog(\"git-daemon\", 0, LOG_DAEMON);\n+\t\topenlog(\"git-daemon\", LOG_PID, LOG_DAEMON);\n \t\tset_die_routine(daemon_die);\n \t}\n+\telse\t\t\t    /* so that logging into a file is atomic */\n+\t\tsetlinebuf(stderr);\n \n \tif (inetd_mode && (group_name || user_name))\n \t\tdie(\"--user and --group are incompatible with --inetd\");\n@@ -1233,8 +1216,10 @@ int main(int argc, char **argv)\n \t\treturn execute(peer);\n \t}\n \n-\tif (detach)\n+\tif (detach) {\n \t\tdaemonize();\n+\t\tloginfo(\"Ready to rumble\");\n+\t}\n \telse\n \t\tsanitize_stdfds();\n \n"},{"id":"87028","messageId":"20080813084331.30845.74788.stgit@aristoteles.cuci.nl","threadId":"14976","inReplyTo":"20080813084330.30845.89753.stgit@aristoteles.cuci.nl","subject":"[PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T08:43:31Z","receivedAt":"2008-08-13T08:43:31Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"by exploiting the fact that systemcalls get interrupted by signals;\nand even they aren't, all zombies will be collected before the next\naccept().\nFix another error() -> logerror() call.\n\nSigned-off-by: Stephen R. van den Berg <srb@cuci.nl>\n---\n\n daemon.c |   67 +++++++++++++++++++++++++++++---------------------------------\n 1 files changed, 31 insertions(+), 36 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 774b2ce..19d620c 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -16,7 +16,6 @@\n static int log_syslog;\n static int verbose;\n static int reuseaddr;\n-static int child_handler_pipe[2];\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n@@ -680,6 +679,21 @@ static void check_dead_children(void)\n {\n \tunsigned spawned, reaped, deleted;\n \n+\tfor (;;) {\n+\t    int status;\n+\t    pid_t pid = waitpid(-1, &status, WNOHANG);\n+\t\n+\t    if (pid > 0) {\n+\t        unsigned reaped = children_reaped;\n+\t        if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t            pid = -pid;\n+\t        dead_child[reaped % MAX_CHILDREN] = pid;\n+\t        children_reaped = reaped + 1;\n+\t        continue;\n+\t    }\n+\t    break;\n+\t}\n+\n \tspawned = children_spawned;\n \treaped = children_reaped;\n \tdeleted = children_deleted;\n@@ -760,21 +774,10 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \n static void child_handler(int signo)\n {\n-\tfor (;;) {\n-\t\tint status;\n-\t\tpid_t pid = waitpid(-1, &status, WNOHANG);\n-\n-\t\tif (pid > 0) {\n-\t\t\tunsigned reaped = children_reaped;\n-\t\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n-\t\t\t\tpid = -pid;\n-\t\t\tdead_child[reaped % MAX_CHILDREN] = pid;\n-\t\t\tchildren_reaped = reaped + 1;\n-\t\t\twrite(child_handler_pipe[1], &status, 1);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tbreak;\n-\t}\n+\t/* Otherwise empty handler because systemcalls will get interrupted\n+\t * upon signal receipt\n+\t * SysV needs the handler to be reinstated\n+\t */\n \tsignal(SIGCHLD, child_handler);\n }\n \n@@ -917,35 +920,30 @@ static int service_loop(int socknum, int *socklist)\n \tstruct pollfd *pfd;\n \tint i;\n \n-\tif (pipe(child_handler_pipe) < 0)\n-\t\tdie (\"Could not set up pipe for child handler\");\n-\n-\tpfd = xcalloc(socknum + 1, sizeof(struct pollfd));\n+\tpfd = xcalloc(socknum, sizeof(struct pollfd));\n \n \tfor (i = 0; i < socknum; i++) {\n \t\tpfd[i].fd = socklist[i];\n \t\tpfd[i].events = POLLIN;\n \t}\n-\tpfd[socknum].fd = child_handler_pipe[0];\n-\tpfd[socknum].events = POLLIN;\n-\n-\tchild_handler(0);\n \n \tfor (;;) {\n \t\tint i;\n+\t\tint olderrno;\n \n-\t\tif (poll(pfd, socknum + 1, -1) < 0) {\n-\t\t\tif (errno != EINTR) {\n-\t\t\t\terror(\"poll failed, resuming: %s\",\n-\t\t\t\t      strerror(errno));\n+\t\ti = poll(pfd, socknum, -1);\n+\t\tolderrno = errno;\n+\n+\t\tcheck_dead_children();\n+\n+\t\tif (i < 0) {\n+\t\t\tif (olderrno != EINTR) {\n+\t\t\t\tlogerror(\"poll failed, resuming: %s\",\n+\t\t\t\t      strerror(olderrno));\n \t\t\t\tsleep(1);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n-\t\tif (pfd[socknum].revents & POLLIN) {\n-\t\t\tread(child_handler_pipe[0], &i, 1);\n-\t\t\tcheck_dead_children();\n-\t\t}\n \n \t\tfor (i = 0; i < socknum; i++) {\n \t\t\tif (pfd[i].revents & POLLIN) {\n@@ -1036,10 +1034,7 @@ int main(int argc, char **argv)\n \tgid_t gid = 0;\n \tint i;\n \n-\t/* Without this we cannot rely on waitpid() to tell\n-\t * what happened to our children.\n-\t */\n-\tsignal(SIGCHLD, SIG_DFL);\n+\tchild_handler(0);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tchar *arg = argv[i];\n"},{"id":"87029","messageId":"20080813085851.GD12628@cuci.nl","threadId":"14976","inReplyTo":"20080813084331.30845.21129.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH 3/3] git-daemon: rewrite kindergarden","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T08:58:51Z","receivedAt":"2008-08-13T08:58:51Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":">+\t\t\tif (max_connections<=0)\n>+\t\t\t\tmax_connections=0;\t        /* unlimited */\n\nHmmm, slight formatting error.  Sorry about that.  I'll resend this\nlast patch.\n-- \nSincerely,\n           Stephen R. van den Berg.\n\n\"And now for something *completely* different!\"\n"},{"id":"87030","messageId":"20080813090040.5176.32630.stgit@aristoteles.cuci.nl","threadId":"14976","inReplyTo":"20080813084331.30845.21129.stgit@aristoteles.cuci.nl","subject":"[PATCH] git-daemon: rewrite kindergarden","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T09:00:40Z","receivedAt":"2008-08-13T09:00:40Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Get rid of the silly fixed array of children and make\nmax-connections dynamic and configurable in the process.\nFix the killing code to actually be smart instead of the\npseudo-random mess.\nAvoid forking if too busy already.\n\nSigned-off-by: Stephen R. van den Berg <srb@cuci.nl>\n---\n\n Documentation/git-daemon.txt |    9 +-\n daemon.c                     |  215 +++++++++++++++---------------------------\n 2 files changed, 84 insertions(+), 140 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 4ba4b75..b08a08c 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -9,8 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git daemon' [--verbose] [--syslog] [--export-all]\n-\t     [--timeout=n] [--init-timeout=n] [--strict-paths]\n-\t     [--base-path=path] [--user-path | --user-path=path]\n+\t     [--timeout=n] [--init-timeout=n] [--max-connections=n]\n+\t     [--strict-paths] [--base-path=path] [--base-path-relaxed]\n+\t     [--user-path | --user-path=path]\n \t     [--interpolated-path=pathtemplate]\n \t     [--reuseaddr] [--detach] [--pid-file=file]\n \t     [--enable=service] [--disable=service]\n@@ -99,6 +100,10 @@ OPTIONS\n \tit takes for the server to process the sub-request and time spent\n \twaiting for next client's request.\n \n+--max-connections::\n+\tMaximum number of concurrent clients, defaults to 32.  Set it to\n+\tzero for no limit.\n+\n --syslog::\n \tLog to syslog instead of stderr. Note that this option does not imply\n \t--verbose, thus by default only error conditions will be logged.\ndiff --git a/daemon.c b/daemon.c\nindex 19d620c..a04ad1c 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -19,8 +19,8 @@ static int reuseaddr;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n-\"           [--timeout=n] [--init-timeout=n] [--strict-paths]\\n\"\n-\"           [--base-path=path] [--base-path-relaxed]\\n\"\n+\"           [--timeout=n] [--init-timeout=n] [--max-connections=n]\\n\"\n+\"           [--strict-paths] [--base-path=path] [--base-path-relaxed]\\n\"\n \"           [--user-path | --user-path=path]\\n\"\n \"           [--interpolated-path=path]\\n\"\n \"           [--reuseaddr] [--detach] [--pid-file=file]\\n\"\n@@ -584,40 +584,37 @@ static int execute(struct sockaddr *addr)\n \treturn -1;\n }\n \n+static int max_connections = 32;\n \n-/*\n- * We count spawned/reaped separately, just to avoid any\n- * races when updating them from signals. The SIGCHLD handler\n- * will only update children_reaped, and the fork logic will\n- * only update children_spawned.\n- *\n- * MAX_CHILDREN should be a power-of-two to make the modulus\n- * operation cheap. It should also be at least twice\n- * the maximum number of connections we will ever allow.\n- */\n-#define MAX_CHILDREN 128\n-\n-static int max_connections = 25;\n-\n-/* These are updated by the signal handler */\n-static volatile unsigned int children_reaped;\n-static pid_t dead_child[MAX_CHILDREN];\n-\n-/* These are updated by the main loop */\n-static unsigned int children_spawned;\n-static unsigned int children_deleted;\n+static unsigned int live_children;\n \n static struct child {\n+\tstruct child*next;\n \tpid_t pid;\n-\tint addrlen;\n \tstruct sockaddr_storage address;\n-} live_child[MAX_CHILDREN];\n+} *firstborn;\n \n-static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n+static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n {\n-\tlive_child[idx].pid = pid;\n-\tlive_child[idx].addrlen = addrlen;\n-\tmemcpy(&live_child[idx].address, addr, addrlen);\n+\tstruct child*newborn;\n+\tnewborn = xcalloc(1, sizeof *newborn);\n+\tif (newborn) {\n+\t\tstruct child **cradle, *blanket;\n+\n+\t\tlive_children++;\n+\t\tnewborn->pid = pid;\n+\t\tmemcpy(&newborn->address, addr, addrlen);\n+\t\tfor (cradle = &firstborn;\n+\t\t     (blanket = *cradle);\n+\t\t     cradle = &blanket->next)\n+\t\t\tif (!memcmp(&blanket->address, &newborn->address,\n+\t\t\t\t   sizeof newborn->address))\n+\t\t\t\tbreak;\n+\t\tnewborn->next = blanket;\n+\t\t*cradle = newborn;\n+\t}\n+\telse\n+\t\tlogerror(\"Out of memory spawning new child\");\n }\n \n /*\n@@ -626,142 +623,78 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n  * We move everything up by one, since the new \"deleted\" will\n  * be one higher.\n  */\n-static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)\n+static void remove_child(pid_t pid)\n {\n-\tstruct child n;\n+\tstruct child **cradle, *blanket;\n \n-\tdeleted %= MAX_CHILDREN;\n-\tspawned %= MAX_CHILDREN;\n-\tif (live_child[deleted].pid == pid) {\n-\t\tlive_child[deleted].pid = -1;\n-\t\treturn;\n-\t}\n-\tn = live_child[deleted];\n-\tfor (;;) {\n-\t\tstruct child m;\n-\t\tdeleted = (deleted + 1) % MAX_CHILDREN;\n-\t\tif (deleted == spawned)\n-\t\t\tdie(\"could not find dead child %d\\n\", pid);\n-\t\tm = live_child[deleted];\n-\t\tlive_child[deleted] = n;\n-\t\tif (m.pid == pid)\n-\t\t\treturn;\n-\t\tn = m;\n-\t}\n+\tfor (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)\n+\t\tif (blanket->pid == pid) {\n+\t\t\t*cradle = blanket->next;\n+\t\t\tlive_children--;\n+\t\t\tfree(blanket);\n+\t\t\tbreak;\n+\t\t}\n }\n \n /*\n  * This gets called if the number of connections grows\n  * past \"max_connections\".\n  *\n- * We _should_ start off by searching for connections\n- * from the same IP, and if there is some address wth\n- * multiple connections, we should kill that first.\n- *\n- * As it is, we just \"randomly\" kill 25% of the connections,\n- * and our pseudo-random generator sucks too. I have no\n- * shame.\n- *\n- * Really, this is just a place-holder for a _real_ algorithm.\n+ * We kill the newest connection from a duplicate IP.\n  */\n-static void kill_some_children(int signo, unsigned start, unsigned stop)\n+static void kill_some_child()\n {\n-\tstart %= MAX_CHILDREN;\n-\tstop %= MAX_CHILDREN;\n-\twhile (start != stop) {\n-\t\tif (!(start & 3))\n-\t\t\tkill(live_child[start].pid, signo);\n-\t\tstart = (start + 1) % MAX_CHILDREN;\n-\t}\n-}\n-\n-static void check_dead_children(void)\n-{\n-\tunsigned spawned, reaped, deleted;\n-\n-\tfor (;;) {\n-\t    int status;\n-\t    pid_t pid = waitpid(-1, &status, WNOHANG);\n-\t\n-\t    if (pid > 0) {\n-\t        unsigned reaped = children_reaped;\n-\t        if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n-\t            pid = -pid;\n-\t        dead_child[reaped % MAX_CHILDREN] = pid;\n-\t        children_reaped = reaped + 1;\n-\t        continue;\n-\t    }\n-\t    break;\n-\t}\n+\tconst struct child *blanket;\n \n-\tspawned = children_spawned;\n-\treaped = children_reaped;\n-\tdeleted = children_deleted;\n+\tif ((blanket = firstborn)) {\n+\t\tconst struct child *next;\n \n-\twhile (deleted < reaped) {\n-\t\tpid_t pid = dead_child[deleted % MAX_CHILDREN];\n-\t\tconst char *dead = pid < 0 ? \" (with error)\" : \"\";\n-\n-\t\tif (pid < 0)\n-\t\t\tpid = -pid;\n-\n-\t\t/* XXX: Custom logging, since we don't wanna getpid() */\n-\t\tif (verbose) {\n-\t\t\tif (log_syslog)\n-\t\t\t\tsyslog(LOG_INFO, \"[%d] Disconnected%s\",\n-\t\t\t\t\t\tpid, dead);\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"[%d] Disconnected%s\\n\",\n-\t\t\t\t\t\tpid, dead);\n-\t\t}\n-\t\tremove_child(pid, deleted, spawned);\n-\t\tdeleted++;\n+\t\tfor (; (next = blanket->next); blanket = next)\n+\t\t\tif (!memcmp(&blanket->address, &next->address,\n+\t\t\t\t   sizeof next->address)) {\n+\t\t\t\tkill(blanket->pid, SIGTERM);\n+\t\t\t\tbreak;\n+\t\t\t}\n \t}\n-\tchildren_deleted = deleted;\n }\n \n-static void check_max_connections(void)\n+static void check_dead_children(void)\n {\n-\tfor (;;) {\n-\t\tint active;\n-\t\tunsigned spawned, deleted;\n-\n-\t\tcheck_dead_children();\n-\n-\t\tspawned = children_spawned;\n-\t\tdeleted = children_deleted;\n-\n-\t\tactive = spawned - deleted;\n-\t\tif (active <= max_connections)\n-\t\t\tbreak;\n-\n-\t\t/* Kill some unstarted connections with SIGTERM */\n-\t\tkill_some_children(SIGTERM, deleted, spawned);\n-\t\tif (active <= max_connections << 1)\n-\t\t\tbreak;\n+\tint status;\n+\tpid_t pid;\n \n-\t\t/* If the SIGTERM thing isn't helping use SIGKILL */\n-\t\tkill_some_children(SIGKILL, deleted, spawned);\n-\t\tsleep(1);\n+\twhile ((pid = waitpid(-1, &status, WNOHANG))>0) {\n+\t\tconst char *dead = \"\";\n+\t\tremove_child(pid);\n+\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t\t\tdead = \" (with error)\";\n+\t\tloginfo(\"[%d] Disconnected%s\", (int)pid, dead);\n \t}\n }\n \n static void handle(int incoming, struct sockaddr *addr, int addrlen)\n {\n-\tpid_t pid = fork();\n+\tpid_t pid;\n \n-\tif (pid) {\n-\t\tunsigned idx;\n+\tif (max_connections && live_children >= max_connections) {\n+\t\tkill_some_child();\n+\t\tsleep(1);\t\t\t /* give it some time to die */\n+\t\tcheck_dead_children();\n+\t\tif (live_children >= max_connections) {\n+\t\t\tclose(incoming);\n+\t\t\tlogerror(\"Too many children, dropping connection\");\n+\t\t\treturn;\n+\t\t}\n+\t}\n \n+\tif ((pid = fork())) {\n \t\tclose(incoming);\n-\t\tif (pid < 0)\n+\t\tif (pid < 0) {\n+\t\t\tlogerror(\"Couldn't fork %s\", strerror(errno));\n \t\t\treturn;\n+\t\t}\n \n-\t\tidx = children_spawned % MAX_CHILDREN;\n-\t\tchildren_spawned++;\n-\t\tadd_child(idx, pid, addr, addrlen);\n-\n-\t\tcheck_max_connections();\n+\t\tadd_child(pid, addr, addrlen);\n \t\treturn;\n \t}\n \n@@ -1081,6 +1014,12 @@ int main(int argc, char **argv)\n \t\t\tinit_timeout = atoi(arg+15);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--max-connections=\")) {\n+\t\t\tmax_connections = atoi(arg+18);\n+\t\t\tif (max_connections < 0)\n+\t\t\t\tmax_connections = 0;\t        /* unlimited */\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--strict-paths\")) {\n \t\t\tstrict_paths = 1;\n \t\t\tcontinue;\n"},{"id":"87031","messageId":"20080813090546.GN32184@machine.or.cz","threadId":"14976","inReplyTo":"20080813084331.30845.21129.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH 3/3] git-daemon: rewrite kindergarden","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2008-08-13T09:05:46Z","receivedAt":"2008-08-13T09:05:46Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Wed, Aug 13, 2008 at 10:43:31AM +0200, Stephen R. van den Berg wrote:\n> Get rid of the silly fixed array of children and make\n> max-connections dynamic and configurable in the process.\n> Fix the killing code to actually be smart instead of the\n> pseudo-random mess.\n> Avoid forking if too busy already.\n> \n> Signed-off-by: Stephen R. van den Berg <srb@cuci.nl>\n\nI would somehow mention the string '--max-connections' in the log\nmessage; that is really useful when looking when some option was\nintroduced.\n\n> diff --git a/daemon.c b/daemon.c\n> index 19d620c..a7a735c 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -584,40 +584,37 @@ static int execute(struct sockaddr *addr)\n>  \treturn -1;\n>  }\n>  \n> +static int max_connections = 32;\n>  \n> -/*\n> - * We count spawned/reaped separately, just to avoid any\n> - * races when updating them from signals. The SIGCHLD handler\n> - * will only update children_reaped, and the fork logic will\n> - * only update children_spawned.\n> - *\n> - * MAX_CHILDREN should be a power-of-two to make the modulus\n> - * operation cheap. It should also be at least twice\n> - * the maximum number of connections we will ever allow.\n> - */\n> -#define MAX_CHILDREN 128\n> -\n> -static int max_connections = 25;\n> -\n> -/* These are updated by the signal handler */\n> -static volatile unsigned int children_reaped;\n> -static pid_t dead_child[MAX_CHILDREN];\n> -\n> -/* These are updated by the main loop */\n> -static unsigned int children_spawned;\n> -static unsigned int children_deleted;\n> +static unsigned int live_children;\n>  \n>  static struct child {\n> +\tstruct child*next;\n\nstruct child *next;\n\n>  \tpid_t pid;\n> -\tint addrlen;\n>  \tstruct sockaddr_storage address;\n> -} live_child[MAX_CHILDREN];\n> +} *firstborn;\n>  \n> -static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n> +static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n>  {\n> -\tlive_child[idx].pid = pid;\n> -\tlive_child[idx].addrlen = addrlen;\n> -\tmemcpy(&live_child[idx].address, addr, addrlen);\n> +\tstruct child*newborn;\n\nstruct child *newborn;\n\n> +\tnewborn = xcalloc(1, sizeof *newborn);\n> +\tif (newborn) {\n> +\t\tstruct child **cradle, *blanket;\n> +\n> +\t\tlive_children++;\n> +\t\tnewborn->pid = pid;\n> +\t\tmemcpy(&newborn->address, addr, addrlen);\n> +\t\tfor (cradle = &firstborn;\n> +\t\t     (blanket = *cradle);\n> +\t\t     cradle = &blanket->next)\n> +\t\t\tif (!memcmp(&blanket->address, &newborn->address,\n> +\t\t\t\t   sizeof newborn->address))\n> +\t\t\t\tbreak;\n> +\t\tnewborn->next = blanket;\n> +\t\t*cradle = newborn;\n\nSo, I can always excuse myself by mentioning that it's early in the\nmorning (for me ;-) but what do you actually need the blanket for, in\nthis warm digital world?\n\nThe current for statement is *really* cryptic... What about:\n\n+\t\tstruct child **cradle;\n+\n+\t\tlive_children++;\n+\t\tnewborn->pid = pid;\n+\t\tmemcpy(&newborn->address, addr, addrlen);\n+\t\tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n+\t\t\tif (!memcmp(&(*cradle)->address, &newborn->address,\n+\t\t\t\t   sizeof newborn->address))\n+\t\t\t\tbreak;\n+\t\tnewborn->next = *cradle;\n+\t\t*cradle = newborn;\n\n> +\t}\n> +\telse\n> +\t\tlogerror(\"Out of memory spawning new child\");\n>  }\n>  \n>  /*\n> @@ -626,142 +623,78 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n>   * We move everything up by one, since the new \"deleted\" will\n>   * be one higher.\n>   */\n> -static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)\n> +static void remove_child(pid_t pid)\n>  {\n> -\tstruct child n;\n> +\tstruct child **cradle, *blanket;\n>  \n> +\tfor (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)\n> +\t\tif (blanket->pid == pid) {\n> +\t\t\t*cradle = blanket->next;\n> +\t\t\tlive_children--;\n> +\t\t\tfree(blanket);\n> +\t\t\tbreak;\n> +\t\t}\n>  }\n\nSame here. You just need a temporary variable in the innermost block.\n\n> +static void kill_some_child()\n>  {\n> +\tconst struct child *blanket;\n>  \n> +\tif ((blanket = firstborn)) {\n> +\t\tconst struct child *next;\n>  \n> +\t\tfor (; (next = blanket->next); blanket = next)\n> +\t\t\tif (!memcmp(&blanket->address, &next->address,\n> +\t\t\t\t   sizeof next->address)) {\n> +\t\t\t\tkill(blanket->pid, SIGTERM);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nI think using cradle instead of blanket in this for loop would be more\nconsistent, if perhaps somewhat more morbid.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nThe next generation of interesting software will be done\non the Macintosh, not the IBM PC.  -- Bill Gates\n"},{"id":"87038","messageId":"20080813103738.GG12628@cuci.nl","threadId":"14976","inReplyTo":"20080813090546.GN32184@machine.or.cz","subject":"Re: [PATCH 3/3] git-daemon: rewrite kindergarden","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T10:37:38Z","receivedAt":"2008-08-13T10:37:38Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Petr Baudis wrote:\n>On Wed, Aug 13, 2008 at 10:43:31AM +0200, Stephen R. van den Berg wrote:\n>I would somehow mention the string '--max-connections' in the log\n>message; that is really useful when looking when some option was\n>introduced.\n\nFixed.\n\n>> +\tstruct child*next;\n\n>struct child *next;\n\n>> +\tstruct child*newborn;\n\n>struct child *newborn;\n\nFixed.\n\n>So, I can always excuse myself by mentioning that it's early in the\n>morning (for me ;-) but what do you actually need the blanket for, in\n>this warm digital world?\n\nThis seems to be a case of code restructuring and forgetting to\noptimise.  There was a reason for the blanket before, but I am warming\nup to the idea to go without blanket in this case.\n\n>The current for statement is *really* cryptic... What about:\n\nProbably ok, I'll refactor it accordingly.\n\n>> -static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)\n>> +static void remove_child(pid_t pid)\n>>  {\n>> -\tstruct child n;\n>> +\tstruct child **cradle, *blanket;\n\n>> +\tfor (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)\n>> +\t\tif (blanket->pid == pid) {\n>> +\t\t\t*cradle = blanket->next;\n>> +\t\t\tlive_children--;\n>> +\t\t\tfree(blanket);\n>> +\t\t\tbreak;\n>> +\t\t}\n\n>Same here. You just need a temporary variable in the innermost block.\n\nYes, but here using the blanket eliminates some * operators.\nSo I'd prefer to keep it.\n\n>> +static void kill_some_child()\n>>  {\n>> +\tconst struct child *blanket;\n\n>> +\tif ((blanket = firstborn)) {\n>> +\t\tconst struct child *next;\n\n>> +\t\tfor (; (next = blanket->next); blanket = next)\n>> +\t\t\tif (!memcmp(&blanket->address, &next->address,\n>> +\t\t\t\t   sizeof next->address)) {\n>> +\t\t\t\tkill(blanket->pid, SIGTERM);\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n\n>I think using cradle instead of blanket in this for loop would be more\n>consistent, if perhaps somewhat more morbid.\n\nI kept the cradle for the double indirections (you have to actually\nreach deeper into a cradle), a blanket can be touched without reaching\ndeep, so I'd prefer to keep a blanket here.\n-- \nSincerely,\n           Stephen R. van den Berg.\n\n\"And now for something *completely* different!\"\n"},{"id":"87039","messageId":"20080813104036.789.95699.stgit@aristoteles.cuci.nl","threadId":"14976","inReplyTo":"20080813090040.5176.32630.stgit@aristoteles.cuci.nl","subject":"[PATCH] git-daemon: rewrite kindergarden, new option --max-connections","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T10:40:36Z","receivedAt":"2008-08-13T10:40:36Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Get rid of the silly fixed array of children and make\nmax-connections dynamic and configurable in the process.\nFix the killing code to actually be smart instead of the\npseudo-random mess.\nAvoid forking if too busy already.\n\nSigned-off-by: Stephen R. van den Berg <srb@cuci.nl>\n---\n\n Documentation/git-daemon.txt |    9 +-\n daemon.c                     |  213 +++++++++++++++---------------------------\n 2 files changed, 82 insertions(+), 140 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 4ba4b75..b08a08c 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -9,8 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git daemon' [--verbose] [--syslog] [--export-all]\n-\t     [--timeout=n] [--init-timeout=n] [--strict-paths]\n-\t     [--base-path=path] [--user-path | --user-path=path]\n+\t     [--timeout=n] [--init-timeout=n] [--max-connections=n]\n+\t     [--strict-paths] [--base-path=path] [--base-path-relaxed]\n+\t     [--user-path | --user-path=path]\n \t     [--interpolated-path=pathtemplate]\n \t     [--reuseaddr] [--detach] [--pid-file=file]\n \t     [--enable=service] [--disable=service]\n@@ -99,6 +100,10 @@ OPTIONS\n \tit takes for the server to process the sub-request and time spent\n \twaiting for next client's request.\n \n+--max-connections::\n+\tMaximum number of concurrent clients, defaults to 32.  Set it to\n+\tzero for no limit.\n+\n --syslog::\n \tLog to syslog instead of stderr. Note that this option does not imply\n \t--verbose, thus by default only error conditions will be logged.\ndiff --git a/daemon.c b/daemon.c\nindex 19d620c..05a4328 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -19,8 +19,8 @@ static int reuseaddr;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n-\"           [--timeout=n] [--init-timeout=n] [--strict-paths]\\n\"\n-\"           [--base-path=path] [--base-path-relaxed]\\n\"\n+\"           [--timeout=n] [--init-timeout=n] [--max-connections=n]\\n\"\n+\"           [--strict-paths] [--base-path=path] [--base-path-relaxed]\\n\"\n \"           [--user-path | --user-path=path]\\n\"\n \"           [--interpolated-path=path]\\n\"\n \"           [--reuseaddr] [--detach] [--pid-file=file]\\n\"\n@@ -584,40 +584,35 @@ static int execute(struct sockaddr *addr)\n \treturn -1;\n }\n \n+static int max_connections = 32;\n \n-/*\n- * We count spawned/reaped separately, just to avoid any\n- * races when updating them from signals. The SIGCHLD handler\n- * will only update children_reaped, and the fork logic will\n- * only update children_spawned.\n- *\n- * MAX_CHILDREN should be a power-of-two to make the modulus\n- * operation cheap. It should also be at least twice\n- * the maximum number of connections we will ever allow.\n- */\n-#define MAX_CHILDREN 128\n-\n-static int max_connections = 25;\n-\n-/* These are updated by the signal handler */\n-static volatile unsigned int children_reaped;\n-static pid_t dead_child[MAX_CHILDREN];\n-\n-/* These are updated by the main loop */\n-static unsigned int children_spawned;\n-static unsigned int children_deleted;\n+static unsigned int live_children;\n \n static struct child {\n+\tstruct child *next;\n \tpid_t pid;\n-\tint addrlen;\n \tstruct sockaddr_storage address;\n-} live_child[MAX_CHILDREN];\n+} *firstborn;\n \n-static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n+static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n {\n-\tlive_child[idx].pid = pid;\n-\tlive_child[idx].addrlen = addrlen;\n-\tmemcpy(&live_child[idx].address, addr, addrlen);\n+\tstruct child *newborn;\n+\tnewborn = xcalloc(1, sizeof *newborn);\n+\tif (newborn) {\n+\t\tstruct child **cradle;\n+\n+\t\tlive_children++;\n+\t\tnewborn->pid = pid;\n+\t\tmemcpy(&newborn->address, addr, addrlen);\n+\t\tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n+\t\t\tif (!memcmp(&(*cradle)->address, &newborn->address,\n+\t\t\t\t   sizeof newborn->address))\n+\t\t\t\tbreak;\n+\t\tnewborn->next = *cradle;\n+\t\t*cradle = newborn;\n+\t}\n+\telse\n+\t\tlogerror(\"Out of memory spawning new child\");\n }\n \n /*\n@@ -626,142 +621,78 @@ static void add_child(int idx, pid_t pid, struct sockaddr *addr, int addrlen)\n  * We move everything up by one, since the new \"deleted\" will\n  * be one higher.\n  */\n-static void remove_child(pid_t pid, unsigned deleted, unsigned spawned)\n+static void remove_child(pid_t pid)\n {\n-\tstruct child n;\n+\tstruct child **cradle, *blanket;\n \n-\tdeleted %= MAX_CHILDREN;\n-\tspawned %= MAX_CHILDREN;\n-\tif (live_child[deleted].pid == pid) {\n-\t\tlive_child[deleted].pid = -1;\n-\t\treturn;\n-\t}\n-\tn = live_child[deleted];\n-\tfor (;;) {\n-\t\tstruct child m;\n-\t\tdeleted = (deleted + 1) % MAX_CHILDREN;\n-\t\tif (deleted == spawned)\n-\t\t\tdie(\"could not find dead child %d\\n\", pid);\n-\t\tm = live_child[deleted];\n-\t\tlive_child[deleted] = n;\n-\t\tif (m.pid == pid)\n-\t\t\treturn;\n-\t\tn = m;\n-\t}\n+\tfor (cradle = &firstborn; (blanket = *cradle); cradle = &blanket->next)\n+\t\tif (blanket->pid == pid) {\n+\t\t\t*cradle = blanket->next;\n+\t\t\tlive_children--;\n+\t\t\tfree(blanket);\n+\t\t\tbreak;\n+\t\t}\n }\n \n /*\n  * This gets called if the number of connections grows\n  * past \"max_connections\".\n  *\n- * We _should_ start off by searching for connections\n- * from the same IP, and if there is some address wth\n- * multiple connections, we should kill that first.\n- *\n- * As it is, we just \"randomly\" kill 25% of the connections,\n- * and our pseudo-random generator sucks too. I have no\n- * shame.\n- *\n- * Really, this is just a place-holder for a _real_ algorithm.\n+ * We kill the newest connection from a duplicate IP.\n  */\n-static void kill_some_children(int signo, unsigned start, unsigned stop)\n+static void kill_some_child()\n {\n-\tstart %= MAX_CHILDREN;\n-\tstop %= MAX_CHILDREN;\n-\twhile (start != stop) {\n-\t\tif (!(start & 3))\n-\t\t\tkill(live_child[start].pid, signo);\n-\t\tstart = (start + 1) % MAX_CHILDREN;\n-\t}\n-}\n-\n-static void check_dead_children(void)\n-{\n-\tunsigned spawned, reaped, deleted;\n-\n-\tfor (;;) {\n-\t    int status;\n-\t    pid_t pid = waitpid(-1, &status, WNOHANG);\n-\t\n-\t    if (pid > 0) {\n-\t        unsigned reaped = children_reaped;\n-\t        if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n-\t            pid = -pid;\n-\t        dead_child[reaped % MAX_CHILDREN] = pid;\n-\t        children_reaped = reaped + 1;\n-\t        continue;\n-\t    }\n-\t    break;\n-\t}\n+\tconst struct child *blanket;\n \n-\tspawned = children_spawned;\n-\treaped = children_reaped;\n-\tdeleted = children_deleted;\n+\tif ((blanket = firstborn)) {\n+\t\tconst struct child *next;\n \n-\twhile (deleted < reaped) {\n-\t\tpid_t pid = dead_child[deleted % MAX_CHILDREN];\n-\t\tconst char *dead = pid < 0 ? \" (with error)\" : \"\";\n-\n-\t\tif (pid < 0)\n-\t\t\tpid = -pid;\n-\n-\t\t/* XXX: Custom logging, since we don't wanna getpid() */\n-\t\tif (verbose) {\n-\t\t\tif (log_syslog)\n-\t\t\t\tsyslog(LOG_INFO, \"[%d] Disconnected%s\",\n-\t\t\t\t\t\tpid, dead);\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"[%d] Disconnected%s\\n\",\n-\t\t\t\t\t\tpid, dead);\n-\t\t}\n-\t\tremove_child(pid, deleted, spawned);\n-\t\tdeleted++;\n+\t\tfor (; (next = blanket->next); blanket = next)\n+\t\t\tif (!memcmp(&blanket->address, &next->address,\n+\t\t\t\t   sizeof next->address)) {\n+\t\t\t\tkill(blanket->pid, SIGTERM);\n+\t\t\t\tbreak;\n+\t\t\t}\n \t}\n-\tchildren_deleted = deleted;\n }\n \n-static void check_max_connections(void)\n+static void check_dead_children(void)\n {\n-\tfor (;;) {\n-\t\tint active;\n-\t\tunsigned spawned, deleted;\n-\n-\t\tcheck_dead_children();\n-\n-\t\tspawned = children_spawned;\n-\t\tdeleted = children_deleted;\n-\n-\t\tactive = spawned - deleted;\n-\t\tif (active <= max_connections)\n-\t\t\tbreak;\n-\n-\t\t/* Kill some unstarted connections with SIGTERM */\n-\t\tkill_some_children(SIGTERM, deleted, spawned);\n-\t\tif (active <= max_connections << 1)\n-\t\t\tbreak;\n+\tint status;\n+\tpid_t pid;\n \n-\t\t/* If the SIGTERM thing isn't helping use SIGKILL */\n-\t\tkill_some_children(SIGKILL, deleted, spawned);\n-\t\tsleep(1);\n+\twhile ((pid = waitpid(-1, &status, WNOHANG))>0) {\n+\t\tconst char *dead = \"\";\n+\t\tremove_child(pid);\n+\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t\t\tdead = \" (with error)\";\n+\t\tloginfo(\"[%d] Disconnected%s\", (int)pid, dead);\n \t}\n }\n \n static void handle(int incoming, struct sockaddr *addr, int addrlen)\n {\n-\tpid_t pid = fork();\n+\tpid_t pid;\n \n-\tif (pid) {\n-\t\tunsigned idx;\n+\tif (max_connections && live_children >= max_connections) {\n+\t\tkill_some_child();\n+\t\tsleep(1);\t\t\t /* give it some time to die */\n+\t\tcheck_dead_children();\n+\t\tif (live_children >= max_connections) {\n+\t\t\tclose(incoming);\n+\t\t\tlogerror(\"Too many children, dropping connection\");\n+\t\t\treturn;\n+\t\t}\n+\t}\n \n+\tif ((pid = fork())) {\n \t\tclose(incoming);\n-\t\tif (pid < 0)\n+\t\tif (pid < 0) {\n+\t\t\tlogerror(\"Couldn't fork %s\", strerror(errno));\n \t\t\treturn;\n+\t\t}\n \n-\t\tidx = children_spawned % MAX_CHILDREN;\n-\t\tchildren_spawned++;\n-\t\tadd_child(idx, pid, addr, addrlen);\n-\n-\t\tcheck_max_connections();\n+\t\tadd_child(pid, addr, addrlen);\n \t\treturn;\n \t}\n \n@@ -1081,6 +1012,12 @@ int main(int argc, char **argv)\n \t\t\tinit_timeout = atoi(arg+15);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--max-connections=\")) {\n+\t\t\tmax_connections = atoi(arg+18);\n+\t\t\tif (max_connections < 0)\n+\t\t\t\tmax_connections = 0;\t        /* unlimited */\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--strict-paths\")) {\n \t\t\tstrict_paths = 1;\n \t\t\tcontinue;\n"},{"id":"87123","messageId":"7vmyjgjyoz.fsf@gitster.siamese.dyndns.org","threadId":"14976","inReplyTo":"20080813084330.30845.89753.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH 1/3] git-daemon: logging done right","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-13T23:13:32Z","receivedAt":"2008-08-13T23:13:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n\n> Make git-daemon a proper syslogging citizen with PID-info.\n\nIn general, please try to avoid using the wording like that in the commit\nlog message, _without_ defining what you think is the proper way and why\nyou think the current behaviour is improper.\n\nDo you mean \"if we use syslog(), we can ask it to add pid information to\nthe output and we do not have to prepend pid information ourselves\"?\n\nFor the same reason, \"logging done right\" is somewhat a suboptimal title\nfor the patch.  If the title describes concisely the reason why the new\nway was chosen by you over the old way, and the readers can judge for\nthemselves if they agree with your reasoning and if the new way is better.\nFor example, you could have said \"git-daemon: let syslog() to add our pid\nto the messages\".\n\nYes, it is _not_ the only change you are making with this patch, and the\nexample message won't describe what you did fully.  It may be an\nindication that you are doing too many things in one patch, unless other\nchanges are \"well, they are not a big deal but while we are at it why not\nfix them\" kind of changes.\n\n> Simplify the overzealous double buffering in the logroutine.\n\nIs there any overzealous double buffering involved?  I thought it just\ndoes s*printf() twice into the single buf[]?  Are you referring to the\ntrick of setting stderr to line-buffered output?  It does remove the need\nfor these two s*printf(), and is an improvement.\n\nI am however not as sure as you seem to be that these two changes make the\ndifference between \"done right\" and \"done wrong\" --- at most I'd say that\nthese fall into \"improve the way the log is done\" category.\n\n> Call logerror() instead of error().\n\nCalls to die() are covered by setting die-routine to daemon_die(), but the\nerror() calls are lost to bitbucket.  This is a real fix to keep otherwise\nlost error message to the log stream.\n\nDon't you want to also send the \"poll failed\" error to the log stream as\nwell?\n\nThanks.\n"},{"id":"87127","messageId":"7v1w0sjw47.fsf@gitster.siamese.dyndns.org","threadId":"14976","inReplyTo":"20080813084331.30845.74788.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-14T00:09:12Z","receivedAt":"2008-08-14T00:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n\n> by exploiting the fact that systemcalls get interrupted by signals;\n> and even they aren't, all zombies will be collected before the next\n> accept().\n\nDscho may want to say something about \"even they aren't...\" part, after he\ncomes back to the keyboard.\n\n> Fix another error() -> logerror() call.\n\nwhich should have been in 1/3, I suppose.\n\n> @@ -1036,10 +1034,7 @@ int main(int argc, char **argv)\n>  \tgid_t gid = 0;\n>  \tint i;\n>  \n> -\t/* Without this we cannot rely on waitpid() to tell\n> -\t * what happened to our children.\n> -\t */\n> -\tsignal(SIGCHLD, SIG_DFL);\n> +\tchild_handler(0);\n\nWhy?\n\nchild_handler() logically is a two step process:\n\n * We are just informed that somebody died; let's do something about the\n   corpse;\n\n * On some systems we need to rearm signals once they fired, so let's do\n   that if necessary.\n\nWith your change, the first part happens to be almost no-op, but I do not\nthink it justifies this hunk.\n\nAfter all, we might even want to do something like:\n\n\tstatic void child_handler(int signo)\n        {\n        \tif (USE_SYSV_SIGNAL_SEMANTICS)\n                \tsignal(SIGCHLD, child_handler);\n\t}\n\nand have the compiler optimize out the signal rearming with\n\n\tcc CFLAGS=-DUSE_SYSV_SIGNAL_SEMANTICS=0\n\non suitable platforms in the future.  But you still want the initial\nsignal set-up to happen unconditionally.\n\nAt this point, we aren't informed by the system that somebody died, and we\nwould want to arm the signal regardless of the platform's signal semantics.\n\nThe rest of the patch looked sane, although I did not read it very\ncarefully.\n\nThanks.\n"},{"id":"87129","messageId":"20080814001858.GB14939@cuci.nl","threadId":"14976","inReplyTo":"7v1w0sjw47.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-14T00:18:58Z","receivedAt":"2008-08-14T00:18:58Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n>> by exploiting the fact that systemcalls get interrupted by signals;\n>> and even they aren't, all zombies will be collected before the next\n>> accept().\n\n>Dscho may want to say something about \"even they aren't...\" part, after he\n>comes back to the keyboard.\n\nThat should have read \"even if they aren't\".  Nonetheless, I don't know\nsystems where it doesn't work this way, but even if a system resisted,\nthe problem is mitigated by the fact that we reap the children before\nevery accept.\n\n>> Fix another error() -> logerror() call.\n\n>which should have been in 1/3, I suppose.\n\nSort of, yes, it was a bit messy to get it out in one piece.\n\n>> @@ -1036,10 +1034,7 @@ int main(int argc, char **argv)\n>>  \tgid_t gid = 0;\n>>  \tint i;\n\n>> -\t/* Without this we cannot rely on waitpid() to tell\n>> -\t * what happened to our children.\n>> -\t */\n>> -\tsignal(SIGCHLD, SIG_DFL);\n>> +\tchild_handler(0);\n\n>Why?\n\nchild_handler() now does barely more than setup the signal handler,\nwhich is exactly what we want to do here.\n\n>With your change, the first part happens to be almost no-op, but I do not\n>think it justifies this hunk.\n\n>After all, we might even want to do something like:\n\n>\tstatic void child_handler(int signo)\n>        {\n>        \tif (USE_SYSV_SIGNAL_SEMANTICS)\n>                \tsignal(SIGCHLD, child_handler);\n\n>and have the compiler optimize out the signal rearming with\n\n>\tcc CFLAGS=-DUSE_SYSV_SIGNAL_SEMANTICS=0\n\nIn return I ask: why?\nThere is no particular performance reason to optimise this.\nSo in order to keep the code simpler, it might make an extra unneeded\nsystemcall on some systems when the signal is processed.  I don't think\nit's worth our while to optimise this further.\n-- \nSincerely,\n           Stephen R. van den Berg.\n\n\"And now for something *completely* different!\"\n"},{"id":"87135","messageId":"7v63q4ieqw.fsf@gitster.siamese.dyndns.org","threadId":"14976","inReplyTo":"20080814001858.GB14939@cuci.nl","subject":"Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-14T01:09:43Z","receivedAt":"2008-08-14T01:09:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n\n>>> @@ -1036,10 +1034,7 @@ int main(int argc, char **argv)\n>>>  \tgid_t gid = 0;\n>>>  \tint i;\n>\n>>> -\t/* Without this we cannot rely on waitpid() to tell\n>>> -\t * what happened to our children.\n>>> -\t */\n>>> -\tsignal(SIGCHLD, SIG_DFL);\n>>> +\tchild_handler(0);\n>\n>>Why?\n>\n> child_handler() now does barely more than setup the signal handler,\n> which is exactly what we want to do here.\n>\n>>With your change, the first part happens to be almost no-op, but I do not\n>>think it justifies this hunk.\n>\n>>After all, we might even want to do something like:\n>\n>>\tstatic void child_handler(int signo)\n>>        {\n>>        \tif (USE_SYSV_SIGNAL_SEMANTICS)\n>>                \tsignal(SIGCHLD, child_handler);\n>\n>>and have the compiler optimize out the signal rearming with\n>\n>>\tcc CFLAGS=-DUSE_SYSV_SIGNAL_SEMANTICS=0\n>\n> In return I ask: why?\n\nPlease read the part you omitted from your quote again.\n\nI agree it would be very meaningless change as an optimization, but I am\nconcerned more about robustness and what makes sense.\n\nDo you agree that \"child_handler()\" is a signal handler to handle SIGCHLD,\nand such a signal handler conceptually consists of two parts? i.e.\n\n\tstatic void child_handler()\n        {\n        \treap_dead_children();\n                rearm_signal_as_needed();\n\t}\n\nYour argument is it is Ok to call this function when you are arming the\nsignal for the first time, because reap_dead_children() happens to be\nempty, and your rearm_signal_as_needed() happens to be the same as\narm_signal_always().\n\nYes, it happens to be _Ok_ now.  But is it an improvement in the longer\nterm?  I do not think so.\n\nI do not see why you think it is better to rely on these two assumptions\nthan being explicit and say \"we set up the signal for the first time\nalways on any platform\", especially when the latter is much more direct\nway to say what your intention is.  Or are you gaining something by not\nexplicitly calling signal() for the first time?  I may be missing some\nbenefit for doing so.\n\nIt is a trade-off between that some benefit I am not seeing, and downside\nthat your version can be broken more easily by future changes to\nchild_handler(), because you are assuming more about what it happens to do\ncurrently.\n\nThat's the kind of thing maintainers worry more about.\n"},{"id":"87164","messageId":"20080814074717.GC9680@cuci.nl","threadId":"14976","inReplyTo":"7v63q4ieqw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-14T07:47:17Z","receivedAt":"2008-08-14T07:47:17Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n>>>> -\tsignal(SIGCHLD, SIG_DFL);\n>>>> +\tchild_handler(0);\n\n>>>After all, we might even want to do something like:\n\n>>>\tstatic void child_handler(int signo)\n>>>        {\n>>>        \tif (USE_SYSV_SIGNAL_SEMANTICS)\n>>>                \tsignal(SIGCHLD, child_handler);\n\n>> In return I ask: why?\n\n>I agree it would be very meaningless change as an optimization, but I am\n>concerned more about robustness and what makes sense.\n\nWell, even if robustness and \"the principle of least surprise\" are\nthe prime concerns, my change could still be considered worthwhile, but\nit depends on your viewpoint.\n\n>Do you agree that \"child_handler()\" is a signal handler to handle SIGCHLD,\n>and such a signal handler conceptually consists of two parts? i.e.\n\nYes.\n\n>\tstatic void child_handler()\n>        {\n>        \treap_dead_children();\n>                rearm_signal_as_needed();\n\n>Your argument is it is Ok to call this function when you are arming the\n>signal for the first time, because reap_dead_children() happens to be\n>empty, and your rearm_signal_as_needed() happens to be the same as\n>arm_signal_always().\n\nWell, not quite, that is part of the argument, the other parts are\nimplicit.\n\n>Yes, it happens to be _Ok_ now.  But is it an improvement in the longer\n>term?  I do not think so.\n\n>I do not see why you think it is better to rely on these two assumptions\n>than being explicit and say \"we set up the signal for the first time\n>always on any platform\", especially when the latter is much more direct\n>way to say what your intention is.\n\nRenaming the function could do it.\n\n>  Or are you gaining something by not\n>explicitly calling signal() for the first time?  I may be missing some\n>benefit for doing so.\n\nWell, strictly speaking the benefits you're overlooking is:\n\n It centralises the spot where the systemcall is made to arm the\n handler.  This means that if the setup needed to arm the handler\n ever becomes more complicated in future OSes, it only needs to be\n updated in one place.  This is a direct maintainability benefit.\n\n>It is a trade-off between that some benefit I am not seeing, and downside\n>that your version can be broken more easily by future changes to\n>child_handler(), because you are assuming more about what it happens to do\n>currently.\n\n>That's the kind of thing maintainers worry more about.\n\nWell, I see two solutions which increase maintainability:\n\nSolution A:\n==================================\nvoid child_handler(int signo) {\n  signal(SIGCHLD, child_handler);\t/* rearm always for portability */\n}\n\nmain() {\n  ...\n  signal(SIGCHLD, child_handler);\n  ...\n}\n==================================\n\nSolution B:\n==================================\nvoid setup_child_handler(int signo) {\n  signal(SIGCHLD, setup_child_handler);\t/* rearm always for portability */\n}\n\nmain() {\n  ...\n  setup_child_handler(0);\n  ...\n}\n==================================\n\nSolution C:\n==================================\nvoid setup_child_handler(void);\n\nvoid child_handler(int signo) {\n  setup_child_handler();\t\t/* rearm always for portability */\n}\n\nvoid setup_child_handler(void) {\n  signal(SIGCHLD, child_handler);\n}\n\nmain() {\n  ...\n  setup_child_handler();\n  ...\n}\n==================================\n\nSolution A is what you propose, but which I find less appealing because\nany future magic to actually setup the handler needs to be maintained\nand updated in two places.\n\nSolution C is what follows your train of thought better, because it\nfuture-proofs the setup as well as the handler.\n\nSolution B is what I consider most elegant and maintainable, because at\nthis point in time I cannot imagine what extra handling would be\nrequired inside the handler which would require a setup as complicated\nas solution C; so in order to keep it as simple as possible and\neliminate forward declarations and minimise systemcalls I suggest we\npick solution B until the need for solution C ever arises (I don't\nthink it ever will).\n\nBut, in any case, you're the maintainer here, not I, so it's your call.\nI vote for B, but just tell me which solution you prefer and I'll adapt\nthe code?\n-- \nSincerely,\n           Stephen R. van den Berg.\n\n\"Hold still, while I inject you with SQL.\"\n"},{"id":"87167","messageId":"7vljz0dmtj.fsf@gitster.siamese.dyndns.org","threadId":"14976","inReplyTo":"20080814074717.GC9680@cuci.nl","subject":"Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-14T08:26:32Z","receivedAt":"2008-08-14T08:26:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n\n> Solution A is what you propose, but which I find less appealing because\n> any future magic to actually setup the handler needs to be maintained\n> and updated in two places.\n\nWell, A is attractive because it leaves the window open for us to later\nnot rearming it unconditionally from child_handler().  I happen to think\nthat the interface we will use to call \"signal()\" is much less likely to\nchange than what we would do in child_handler().\n\n> Solution C is what follows your train of thought better, because it\n> future-proofs the setup as well as the handler.\n\nSurely, we could take this route as the logical conclusion from my\nmaintainability concerns, except that, if we are to make things as fine\ngrained as you suggest, we would definitely will not have a single\n\"setup\", but \"setup\" and \"rearm\" as separate functions.  Perhaps \"setup\"\nmay start using sigaction() with SA_RESETHAND cleared, and we can make\n\"rearm\" truly a no-op everywhere.\n\n \n"},{"id":"87174","messageId":"20080814101307.GG9680@cuci.nl","threadId":"14976","inReplyTo":"7vljz0dmtj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-14T10:13:07Z","receivedAt":"2008-08-14T10:13:07Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n>> Solution A is what you propose, but which I find less appealing because\n>> any future magic to actually setup the handler needs to be maintained\n>> and updated in two places.\n\n>Well, A is attractive because it leaves the window open for us to later\n>not rearming it unconditionally from child_handler().  I happen to think\n>that the interface we will use to call \"signal()\" is much less likely to\n>change than what we would do in child_handler().\n\nThen that is the fundamental difference in maintainability-views.\nI find it more likely that we change the interface to setup the signal\nhandler (in a future OS or by changing the way you invoke it), than \nthat the actual work done by the signal handler is changed.\n\nBut since your view carries more weight here, I'll adapt the patch to\nsolution A.\n\n>> Solution C is what follows your train of thought better, because it\n>> future-proofs the setup as well as the handler.\n\n>\"setup\", but \"setup\" and \"rearm\" as separate functions.  Perhaps \"setup\"\n>may start using sigaction() with SA_RESETHAND cleared, and we can make\n>\"rearm\" truly a no-op everywhere.\n\n>From a portability standpoint using sigaction is worse than signal, and\nrearming the handler even when it is not required is not a big deal.\n-- \nSincerely,\n           Stephen R. van den Berg.\n\n\"Hold still, while I inject you with SQL.\"\n"},{"id":"87189","messageId":"alpine.DEB.1.00.0808141539210.24820@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"14976","inReplyTo":"20080813084331.30845.74788.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH 2/3] git-daemon: make the signal handler almost a no-op","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-08-14T13:41:35Z","receivedAt":"2008-08-14T13:41:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 13 Aug 2008, Stephen R. van den Berg wrote:\n\n> by exploiting the fact that systemcalls get interrupted by signals; and \n> even they aren't, all zombies will be collected before the next \n> accept(). Fix another error() -> logerror() call.\n\nI find this commit message more than cryptic.  So much so that I would \nhave to study the patch in detail to _understand_ what it does.\n\nNot having the time, I refuse,\nDscho\n\nP.S.: Oh, and I might have been a little appalled by the language of your \ncommit messages.\n"}]}