{"thread":{"id":"14966","subject":"[PATCH] git-daemon: SysV needs the signal handler reinstated.","startedAt":"2008-08-12T19:36:13Z","lastAt":"2008-08-13T19:28:55Z","messageCount":17,"participants":["Stephen R. van den Berg","Junio C Hamano","Alex Riesen","Marcus Griep","Andreas Ericsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"86964","messageId":"20080812193613.32388.92145.stgit@aristoteles.cuci.nl","threadId":"14966","inReplyTo":null,"subject":"[PATCH] git-daemon: SysV needs the signal handler reinstated.","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-12T19:36:13Z","receivedAt":"2008-08-12T19:36:13Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Fixes the bug on (amongst others) Solaris that only the first\nchild ever is reaped.\n\nSigned-off-by: Stephen R. van den Berg \n---\n\n daemon.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 4540e8d..1c00305 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -794,6 +794,7 @@ static void child_handler(int signo)\n \t\t}\n \t\tbreak;\n \t}\n+\tsignal(SIGCHLD, child_handler);\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -947,7 +948,7 @@ static int service_loop(int socknum, int *socklist)\n \tpfd[socknum].fd = child_handler_pipe[0];\n \tpfd[socknum].events = POLLIN;\n \n-\tsignal(SIGCHLD, child_handler);\n+\tchild_handler(0);\n \n \tfor (;;) {\n \t\tint i;\n"},{"id":"86974","messageId":"20080812212534.6871.19377.stgit@aristoteles.cuci.nl","threadId":"14966","inReplyTo":"20080812193613.32388.92145.stgit@aristoteles.cuci.nl","subject":"[PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-12T21:25:35Z","receivedAt":"2008-08-12T21:25:35Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"making the signal handler almost a no-op.\nFix the killing code to actually be smart instead of the\npseudo-random mess.\nGet rid of the silly fixed array of children and make\nmax-connections dynamic and configurable in the process.\nMake git-daemon a proper syslogging citizen with PID-info.\nSimplify the overzealous double buffering in the logroutine,\nremove the artificial maximum logline length in the process.\n\nSigned-off-by: Stephen R. van den Berg \n---\n\nI actually only wanted to fix the signal handler, so that it works\non SysV/Solaris systems (see previous patch).\n\nThen again, once I started looking at the code, it's amazing how\nmuch functionality and robustness can be *improved* by actually\nremoving more and more code.\n\n Documentation/git-daemon.txt | 8 +\n daemon.c | 277 +++++++++++++++---------------------------\n 2 files changed, 108 insertions(+), 177 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 4ba4b75..48bcc25 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,9 @@ 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.\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 1c00305..8e67747 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -16,12 +16,11 @@\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-\" [--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@@ -78,38 +77,16 @@ 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\tsyslog(priority, \"%s\", buf);\n-\t\treturn;\n+\tif (log_syslog)\n+\t\tvsyslog(priority, err, params);\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-\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 }\n \n static void logerror(const char *err, ...)\n@@ -605,39 +582,38 @@ static int execute(struct sockaddr *addr)\n }\n \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+static int max_connections = 32;\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+ struct 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+ struct child*newborn;\n+ newborn = xmalloc(sizeof *newborn);\n+ if (newborn) {\n+\t struct child**cradle,*blanket;\n+\n+\t\tlive_children++;\n+\t\tnewborn->pid = pid;\n+\t\tmemcpy(memset(&newborn->address, 0, sizeof newborn->address),\n+\t\t 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+ }\n+\telse\n+\t\tlogerror(\"Out of memory spawning new child\");\n }\n \n /*\n@@ -646,107 +622,74 @@ 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-\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+\tstruct child**cradle,*blanket;\n+\n+\tfor (cradle = &firstborn;\n+\t (blanket = *cradle);\n+\t 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 first.\n+ * If there are no duplicates, then the newest connection is killed,\n+ * in order to allow long running clones to actually complete.\n */\n-static void kill_some_children(int signo, unsigned start, unsigned stop)\n+static void kill_some_child(int signo)\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+\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\tbreak;\n+\n+\t\tkill(blanket->pid, signo);\n \t}\n }\n \n static void check_dead_children(void)\n {\n-\tunsigned spawned, reaped, deleted;\n-\n-\tspawned = children_spawned;\n-\treaped = children_reaped;\n-\tdeleted = children_deleted;\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+\tint status;\n+\tpid_t 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+\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-\tchildren_deleted = deleted;\n }\n \n static void check_max_connections(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\tif (live_children <= 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\tkill_some_child(SIGTERM);\n+\t\tcheck_dead_children();\n+\t\tif (live_children <= max_connections << 1)\n \t\t\tbreak;\n \n \t\t/* If the SIGTERM thing isn't helping use SIGKILL */\n-\t\tkill_some_children(SIGKILL, deleted, spawned);\n+\t\tkill_some_child(SIGKILL);\n \t\tsleep(1);\n \t}\n }\n@@ -756,16 +699,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \tpid_t pid = fork();\n \n \tif (pid) {\n-\t\tunsigned idx;\n-\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\tadd_child(pid, addr, addrlen);\n \t\tcheck_max_connections();\n \t\treturn;\n \t}\n@@ -779,21 +719,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+ * upon signal receipt\n+ * SysV needs the handler to be reinstated\n+ */\n \tsignal(SIGCHLD, child_handler);\n }\n \n@@ -936,35 +865,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\ti = poll(pfd, socknum, -1);\n+\t\tolderrno = errno;\n \n-\t\tif (poll(pfd, socknum + 1, -1) < 0) {\n-\t\t\tif (errno != EINTR) {\n+\t\tcheck_dead_children();\n+\n+\t\tif (i < 0) {\n+\t\t\tif (olderrno != EINTR) {\n \t\t\t\terror(\"poll failed, resuming: %s\",\n-\t\t\t\t strerror(errno));\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@@ -1055,10 +979,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@@ -1105,6 +1026,10 @@ 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\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--strict-paths\")) {\n \t\t\tstrict_paths = 1;\n \t\t\tcontinue;\n@@ -1178,9 +1103,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"},{"id":"86978","messageId":"7vzlnhq48b.fsf@gitster.siamese.dyndns.org","threadId":"14966","inReplyTo":"20080812212534.6871.19377.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-12T22:05:08Z","receivedAt":"2008-08-12T22:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" writes:\n\n> making the signal handler almost a no-op.\n> Fix the killing code to actually be smart instead of the\n> pseudo-random mess.\n> Get rid of the silly fixed array of children and make\n> max-connections dynamic and configurable in the process.\n> Make git-daemon a proper syslogging citizen with PID-info.\n> Simplify the overzealous double buffering in the logroutine,\n> remove the artificial maximum logline length in the process.\n>\n> Signed-off-by: Stephen R. van den Berg \n\nSorry, but this does too many things in one patch.\n\n - Taking advantage of poll() getting interrupted by SIGCHLD, so that you\n do not have to do anything in the signal handler, is so obvious that I\n am actually ashamed of not having to think of it the last time we\n touched this code. Is there a poll() that does not return EINTR but\n just call the handler and restart after that as if nothing has\n happened, I have to wonder...\n\n - Conversion from silly fixed array to dynamic and configurable maximum\n would be a good idea, but that is independent from the above, isn't it?\n\n - I see you have a call to vsyslog, which is the first user of the\n function. How portable is it (the patch coming from you, I know\n Solaris would have it, and recent 4BSD also would, but what about the\n others)?\n"},{"id":"86980","messageId":"20080812223224.GA4134@steel.home","threadId":"14966","inReplyTo":"20080812212534.6871.19377.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-08-12T22:32:24Z","receivedAt":"2008-08-12T22:32:24Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Stephen R. van den Berg, Tue, Aug 12, 2008 23:25:35 +0200:\n> + struct child*newborn;\n\nYou may want to reformat the patch using tabs instead of spaces.\n\n> + newborn = xmalloc(sizeof *newborn);\n\nThe custom here is to use \"sizeof(type)\". Brackets and typename.\n\n> + if (newborn) {\n> +\t struct child**cradle,*blanket;\n\n\"struct child **cradle, *blanket;\" (the spaces before asterisks)\n\n> +\t\tmemcpy(memset(&newborn->address, 0, sizeof newborn->address),\n> +\t\t addr, addrlen);\n\nAren't separate calls easier to read (and type)?\n\n\tmemset(&newborn->address, 0, sizeof(newborn->address));\n\tmemcpy(&newborn->address, addr, addrlen);\n\n> -static void kill_some_children(int signo, unsigned start, unsigned stop)\n> +static void kill_some_child(int signo)\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> +\tconst struct child *blanket;\n> +\n> +\tif ((blanket = firstborn)) {\n\n\tif (firstborn) {\n\t const struct child *blanket = firstborn;\n\nYou don't even use blanket outside of the \"if\".\n\n> static void check_dead_children(void)\n> {\n> +\t\tloginfo(\"[%d] Disconnected%s\", (int)pid, dead);\n\nBTW, why do you need that pid_t->int cast?\n\n> @@ -1105,6 +1026,10 @@ 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\nAn error checking wouldn't go amiss. And it can't be done with atoi\n(consider strtol).\n"},{"id":"86982","messageId":"20080812225642.GA15265@cuci.nl","threadId":"14966","inReplyTo":"7vzlnhq48b.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-12T22:56:42Z","receivedAt":"2008-08-12T22:56:42Z","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\" writes:\n>Sorry, but this does too many things in one patch.\n\nYes, I know, got carried away. Then again, the code has a lot of\noverlapping places (spacewise); I kind of leapt from one place to the\nnext; you fix one thing, and then the next wart stares you in the face.\nI'll see if I can split it up, if that suits you better.\n\n> - Taking advantage of poll() getting interrupted by SIGCHLD, so that you\n> do not have to do anything in the signal handler, is so obvious that I\n> am actually ashamed of not having to think of it the last time we\n> touched this code. Is there a poll() that does not return EINTR but\n> just call the handler and restart after that as if nothing has\n> happened, I have to wonder...\n\nOnly if the signal is set to SIG_IGN on all systems I worked with since\n1987.\n\n> - Conversion from silly fixed array to dynamic and configurable maximum\n> would be a good idea, but that is independent from the above, isn't it?\n\nIt is, but the code is on the same lines (in large parts).\nSeparating it causes two things:\na. The patches to become dependent on each other in the timeline.\nb. More (redundant) work, because some parts that need to be rewritten, get\n deleted by the following patch(es).\n\n> - I see you have a call to vsyslog, which is the first user of the\n> function. How portable is it (the patch coming from you, I know\n> Solaris would have it, and recent 4BSD also would, but what about the\n> others)?\n\nCygwin has it, Solaris does, Linux does, MacOSX does.\nAIX and HPUX don't, perhaps.\nI'll see what I can do to avoid it, yet simplify the code.\n-- \nSincerely,\n Stephen R. van den Berg.\n\nFather's Day Special at the local clinic -- Vasectomy!\n"},{"id":"86986","messageId":"20080812231210.GB15265@cuci.nl","threadId":"14966","inReplyTo":"20080812223224.GA4134@steel.home","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-12T23:12:10Z","receivedAt":"2008-08-12T23:12:10Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Alex Riesen wrote:\n>Stephen R. van den Berg, Tue, Aug 12, 2008 23:25:35 +0200:\n>> + struct child*newborn;\n\n>You may want to reformat the patch using tabs instead of spaces.\n\nTried to do it right, but apparently missed a few spots, thanks.\n\n>> + newborn = xmalloc(sizeof *newborn);\n\n>The custom here is to use \"sizeof(type)\". Brackets and typename.\n\nWell, as for the parens, current git source shows a ratio of 26 : 887\nin your favour; apparently not everyone agrees, but most do.\nAs for the typename, that rather uncommon, in most cases actual\nvariables are used (with good reason).\n\n>> + if (newborn) {\n>> +\t struct child**cradle,*blanket;\n\n>\"struct child **cradle, *blanket;\" (the spaces before asterisks)\n\nI'll be more careful.\n\n>> +\t\tmemcpy(memset(&newborn->address, 0, sizeof newborn->address),\n>> +\t\t addr, addrlen);\n\n>Aren't separate calls easier to read (and type)?\n\nPossibly, yes.\n\n>\tmemset(&newborn->address, 0, sizeof(newborn->address));\n>\tmemcpy(&newborn->address, addr, addrlen);\n\nBut it results in more machinecode in most cases. Sorry, is my builtin\nmicro-optimiser at work.\n\n>> -static void kill_some_children(int signo, unsigned start, unsigned stop)\n>> +static void kill_some_child(int signo)\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>> +\tconst struct child *blanket;\n>> +\n>> +\tif ((blanket = firstborn)) {\n\n>\tif (firstborn) {\n>\t const struct child *blanket = firstborn;\n\n>You don't even use blanket outside of the \"if\".\n\nGood thinking. I restructured the code a few times, previously this\nwasn't possible.\n\n>> static void check_dead_children(void)\n>> {\n>> +\t\tloginfo(\"[%d] Disconnected%s\", (int)pid, dead);\n\n>BTW, why do you need that pid_t->int cast?\n\nBecause pid_t might be wider than int (unlikely, but possible),\nand then this call might crash the program.\n\n>> @@ -1105,6 +1026,10 @@ int main(int argc, char **argv)\n>> \t\t\tinit_timeout = atoi(arg+15);\n>> \t\t\tcontinue;\n\n>> +\t\tif (!prefixcmp(arg, \"--max-connections=\")) {\n>> +\t\t\tmax_connections = atoi(arg+18);\n\n>An error checking wouldn't go amiss. And it can't be done with atoi\n>(consider strtol).\n\nI merely copied the other argument parsing methods, didn't want to\nimprove this, just functionally equivalent (will do fine here, IMO).\n-- \nSincerely,\n Stephen R. van den Berg.\n\nFather's Day Special at the local clinic -- Vasectomy!\n"},{"id":"86987","messageId":"20080812235247.8353.95609.stgit@aristoteles.cuci.nl","threadId":"14966","inReplyTo":"20080812225642.GA15265@cuci.nl","subject":"[PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-12T23:52:47Z","receivedAt":"2008-08-12T23:52:47Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"making the signal handler almost a no-op.\nFix the killing code to actually be smart instead of the\npseudo-random mess.\nGet rid of the silly fixed array of children and make\nmax-connections dynamic and configurable in the process.\nMake git-daemon a proper syslogging citizen with PID-info.\nSimplify the overzealous double buffering in the logroutine,\nremove the artificial maximum logline length in the process.\nChanged two calls to error() into logerror().\n\nSigned-off-by: Stephen R. van den Berg \n---\n\nTook out the vsyslog().\nCleanup a bit left and right.\nCan this go in in one piece? Or do I need to split it up?\nLike I said, large parts of the split-up are obsolete due do getting\ndeleted.\n\n Documentation/git-daemon.txt | 8 +\n daemon.c | 285 ++++++++++++++++--------------------------\n 2 files changed, 114 insertions(+), 179 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 4ba4b75..48bcc25 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,9 @@ 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.\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 1c00305..82eb224 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -16,12 +16,11 @@\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-\" [--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@@ -78,38 +77,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@@ -604,40 +584,38 @@ 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 = xmalloc(sizeof *newborn);\n+\tif (newborn) {\n+\t\tstruct child **cradle, *blanket;\n+\n+\t\tlive_children++;\n+\t\tnewborn->pid = pid;\n+\t\tmemcpy(memset(&newborn->address, 0, sizeof newborn->address),\n+\t\t 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@@ -646,107 +624,74 @@ 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-\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+\tstruct child **cradle, *blanket;\n+\n+\tfor (cradle = &firstborn;\n+\t (blanket = *cradle);\n+\t 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 first.\n+ * If there are no duplicates, then the newest connection is killed,\n+ * in order to allow long running clones to actually complete.\n */\n-static void kill_some_children(int signo, unsigned start, unsigned stop)\n+static void kill_some_child(int signo)\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+\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\tbreak;\n+\n+\t\tkill(blanket->pid, signo);\n \t}\n }\n \n static void check_dead_children(void)\n {\n-\tunsigned spawned, reaped, deleted;\n-\n-\tspawned = children_spawned;\n-\treaped = children_reaped;\n-\tdeleted = children_deleted;\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+\tint status;\n+\tpid_t 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+\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-\tchildren_deleted = deleted;\n }\n \n static void check_max_connections(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\tif (live_children <= 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\tkill_some_child(SIGTERM);\n+\t\tcheck_dead_children();\n+\t\tif (live_children <= max_connections << 1)\n \t\t\tbreak;\n \n \t\t/* If the SIGTERM thing isn't helping use SIGKILL */\n-\t\tkill_some_children(SIGKILL, deleted, spawned);\n+\t\tkill_some_child(SIGKILL);\n \t\tsleep(1);\n \t}\n }\n@@ -756,16 +701,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \tpid_t pid = fork();\n \n \tif (pid) {\n-\t\tunsigned idx;\n-\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\tadd_child(pid, addr, addrlen);\n \t\tcheck_max_connections();\n \t\treturn;\n \t}\n@@ -779,21 +721,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@@ -836,7 +767,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@@ -936,35 +867,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\ti = poll(pfd, socknum, -1);\n+\t\tolderrno = errno;\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\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@@ -1055,10 +981,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@@ -1105,6 +1028,10 @@ 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\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--strict-paths\")) {\n \t\t\tstrict_paths = 1;\n \t\t\tcontinue;\n@@ -1178,9 +1105,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 +1162,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":"86988","messageId":"20080813000312.12323.65448.stgit@aristoteles.cuci.nl","threadId":"14966","inReplyTo":"20080812235247.8353.95609.stgit@aristoteles.cuci.nl","subject":"[PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T00:03:12Z","receivedAt":"2008-08-13T00:03:12Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"making the signal handler almost a no-op.\nFix the killing code to actually be smart instead of the\npseudo-random mess.\nGet rid of the silly fixed array of children and make\nmax-connections dynamic and configurable in the process.\nMake git-daemon a proper syslogging citizen with PID-info.\nSimplify the overzealous double buffering in the logroutine,\nremove the artificial maximum logline length in the process.\nChanged two calls to error() into logerror().\n\nSigned-off-by: Stephen R. van den Berg \n---\n\nCleaned up a bit further.\n\n Documentation/git-daemon.txt | 8 +\n daemon.c | 280 ++++++++++++++++--------------------------\n 2 files changed, 110 insertions(+), 178 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 4ba4b75..48bcc25 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,9 @@ 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.\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 1c00305..0de92a8 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -16,12 +16,11 @@\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-\" [--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@@ -78,38 +77,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@@ -604,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@@ -646,107 +623,72 @@ 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 first.\n+ * If there are no duplicates, then the newest connection is killed,\n+ * in order to allow long running clones to actually complete.\n */\n-static void kill_some_children(int signo, unsigned start, unsigned stop)\n+static void kill_some_child(int signo)\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+\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\tbreak;\n+\n+\t\tkill(blanket->pid, signo);\n \t}\n }\n \n static void check_dead_children(void)\n {\n-\tunsigned spawned, reaped, deleted;\n-\n-\tspawned = children_spawned;\n-\treaped = children_reaped;\n-\tdeleted = children_deleted;\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+\tint status;\n+\tpid_t 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+\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-\tchildren_deleted = deleted;\n }\n \n static void check_max_connections(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\tif (live_children <= 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\tkill_some_child(SIGTERM);\n+\t\tcheck_dead_children();\n+\t\tif (live_children <= max_connections << 1)\n \t\t\tbreak;\n \n \t\t/* If the SIGTERM thing isn't helping use SIGKILL */\n-\t\tkill_some_children(SIGKILL, deleted, spawned);\n+\t\tkill_some_child(SIGKILL);\n \t\tsleep(1);\n \t}\n }\n@@ -756,16 +698,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \tpid_t pid = fork();\n \n \tif (pid) {\n-\t\tunsigned idx;\n-\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\tadd_child(pid, addr, addrlen);\n \t\tcheck_max_connections();\n \t\treturn;\n \t}\n@@ -779,21 +718,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@@ -836,7 +764,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@@ -936,35 +864,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\ti = poll(pfd, socknum, -1);\n+\t\tolderrno = errno;\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\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@@ -1055,10 +978,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@@ -1105,6 +1025,10 @@ 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\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--strict-paths\")) {\n \t\t\tstrict_paths = 1;\n \t\t\tcontinue;\n@@ -1178,9 +1102,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 +1159,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":"86989","messageId":"48A225BC.7070709@griep.us","threadId":"14966","inReplyTo":"20080812235247.8353.95609.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Marcus Griep","fromEmail":"marcus@griep.us","sentAt":"2008-08-13T00:07:24Z","receivedAt":"2008-08-13T00:07:24Z","isPatch":true,"sender":{"key":"marcus@griep.us","avatar":"https://gravatar.com/avatar/0a841f2aad3f9a38c9bcf87a567c2a1751cb08924bae1bc46ee171395bcf9794?d=mp&s=160"},"body":"I'd still suggest this get split up. Even though the patches may\nbe dependent upon one another, each logical unit should be it's\nown patch and commit in the Git repository. Submit them as a\npatch series with a summary and it'll make each part clear as\nto what was modified to achieve what, and easier to review.\n\nAlso, the title to your patch is malformed. Perhaps your MUA doing\nsome unintended wrapping?\n\nMarcus\n\nStephen R. van den Berg wrote:\n> making the signal handler almost a no-op.\n> Fix the killing code to actually be smart instead of the\n> pseudo-random mess.\n> Get rid of the silly fixed array of children and make\n> max-connections dynamic and configurable in the process.\n> Make git-daemon a proper syslogging citizen with PID-info.\n> Simplify the overzealous double buffering in the logroutine,\n> remove the artificial maximum logline length in the process.\n> Changed two calls to error() into logerror().\n> \n> Signed-off-by: Stephen R. van den Berg \n> ---\n> \n> Took out the vsyslog().\n> Cleanup a bit left and right.\n> Can this go in in one piece? Or do I need to split it up?\n> Like I said, large parts of the split-up are obsolete due do getting\n> deleted.\n> \n> Documentation/git-daemon.txt | 8 +\n> daemon.c | 285 ++++++++++++++++--------------------------\n> 2 files changed, 114 insertions(+), 179 deletions(-)\n> \n> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n> index 4ba4b75..48bcc25 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,9 @@ 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.\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.\n> diff --git a/daemon.c b/daemon.c\n> index 1c00305..82eb224 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -16,12 +16,11 @@\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> -\" [--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> @@ -78,38 +77,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> @@ -604,40 +584,38 @@ 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 = xmalloc(sizeof *newborn);\n> +\tif (newborn) {\n> +\t\tstruct child **cradle, *blanket;\n> +\n> +\t\tlive_children++;\n> +\t\tnewborn->pid = pid;\n> +\t\tmemcpy(memset(&newborn->address, 0, sizeof newborn->address),\n> +\t\t 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> @@ -646,107 +624,74 @@ 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> -\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> +\tstruct child **cradle, *blanket;\n> +\n> +\tfor (cradle = &firstborn;\n> +\t (blanket = *cradle);\n> +\t 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 first.\n> + * If there are no duplicates, then the newest connection is killed,\n> + * in order to allow long running clones to actually complete.\n> */\n> -static void kill_some_children(int signo, unsigned start, unsigned stop)\n> +static void kill_some_child(int signo)\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> +\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\tbreak;\n> +\n> +\t\tkill(blanket->pid, signo);\n> \t}\n> }\n> \n> static void check_dead_children(void)\n> {\n> -\tunsigned spawned, reaped, deleted;\n> -\n> -\tspawned = children_spawned;\n> -\treaped = children_reaped;\n> -\tdeleted = children_deleted;\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> +\tint status;\n> +\tpid_t 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> +\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> -\tchildren_deleted = deleted;\n> }\n> \n> static void check_max_connections(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\tif (live_children <= 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\tkill_some_child(SIGTERM);\n> +\t\tcheck_dead_children();\n> +\t\tif (live_children <= max_connections << 1)\n> \t\t\tbreak;\n> \n> \t\t/* If the SIGTERM thing isn't helping use SIGKILL */\n> -\t\tkill_some_children(SIGKILL, deleted, spawned);\n> +\t\tkill_some_child(SIGKILL);\n> \t\tsleep(1);\n> \t}\n> }\n> @@ -756,16 +701,13 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n> \tpid_t pid = fork();\n> \n> \tif (pid) {\n> -\t\tunsigned idx;\n> -\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\tadd_child(pid, addr, addrlen);\n> \t\tcheck_max_connections();\n> \t\treturn;\n> \t}\n> @@ -779,21 +721,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> @@ -836,7 +767,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> @@ -936,35 +867,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\ti = poll(pfd, socknum, -1);\n> +\t\tolderrno = errno;\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\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> @@ -1055,10 +981,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> @@ -1105,6 +1028,10 @@ 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\tcontinue;\n> +\t\t}\n> \t\tif (!strcmp(arg, \"--strict-paths\")) {\n> \t\t\tstrict_paths = 1;\n> \t\t\tcontinue;\n> @@ -1178,9 +1105,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 +1162,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> \n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n-- \nMarcus Griep\nGPG Key ID: 0x5E968152\n——\nhttp://www.boohaunt.net\nאת.ψο´\n"},{"id":"86992","messageId":"7vvdy5pxz6.fsf@gitster.siamese.dyndns.org","threadId":"14966","inReplyTo":"20080812225642.GA15265@cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-13T00:20:13Z","receivedAt":"2008-08-13T00:20:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" writes:\n\n> Separating it causes two things:\n> a. The patches to become dependent on each other in the timeline.\n> b. More (redundant) work, because some parts that need to be rewritten, get\n> deleted by the following patch(es).\n\nThese are actually desirable properties from reviewability point of view.\n\n>> - I see you have a call to vsyslog, which is the first user of the\n>> function. How portable is it (the patch coming from you, I know\n>> Solaris would have it, and recent 4BSD also would, but what about the\n>> others)?\n>\n> Cygwin has it, Solaris does, Linux does, MacOSX does.\n> AIX and HPUX don't, perhaps.\n> I'll see what I can do to avoid it, yet simplify the code.\n\nThat's one of the reasons why I asked you to split it to three patches, so\nthat the syslog change can potentially be independently replaced with a\nbetter alternative.\n\nIn any case, it is already late in the rc cycle; I'd like to apply your\nearlier \"In SysV, signal(SIGCHLD) need to be rearmed\" patch and nothing\nelse for now. The clean-up is very attractive but can be done post 1.6.0.\n"},{"id":"87012","messageId":"20080813062008.GA4345@blimp.local","threadId":"14966","inReplyTo":"20080812231210.GB15265@cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-08-13T06:20:08Z","receivedAt":"2008-08-13T06:20:08Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Stephen R. van den Berg, Wed, Aug 13, 2008 01:12:10 +0200:\n> Alex Riesen wrote:\n> >Stephen R. van den Berg, Tue, Aug 12, 2008 23:25:35 +0200:\n> >> @@ -1105,6 +1026,10 @@ int main(int argc, char **argv)\n> >> \t\t\tinit_timeout = atoi(arg+15);\n> >> \t\t\tcontinue;\n> \n> >> +\t\tif (!prefixcmp(arg, \"--max-connections=\")) {\n> >> +\t\t\tmax_connections = atoi(arg+18);\n> \n> >An error checking wouldn't go amiss. And it can't be done with atoi\n> >(consider strtol).\n> \n> I merely copied the other argument parsing methods, didn't want to\n> improve this, just functionally equivalent (will do fine here, IMO).\n\nIn case of error max_connections will be 0. How does your code handle\nit? It is a server side program, which supposed to provide reliable\nservice. In this case, an operator mistake is not even noticable until\nthe first request and even than the clients have to complain first.\n"},{"id":"87013","messageId":"48A28231.6090105@op5.se","threadId":"14966","inReplyTo":"20080812212534.6871.19377.stgit@aristoteles.cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2008-08-13T06:41:53Z","receivedAt":"2008-08-13T06:41:53Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Stephen R. van den Berg wrote:\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\nstderr is unbuffered. It's stdout that is linebuffered.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n"},{"id":"87019","messageId":"81b0412b0808130018n63c71089u52b95f5a836f35f3@mail.gmail.com","threadId":"14966","inReplyTo":"48A28231.6090105@op5.se","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-08-13T07:18:37Z","receivedAt":"2008-08-13T07:18:37Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/8/13 Andreas Ericsson :\n> Stephen R. van den Berg wrote:\n>>\n>> + /* Since stderr is set to linebuffered mode, the\n>> + * logging of different processes will not overlap\n>> + */\n>> + fprintf(stderr, \"[%d] \", (int)getpid());\n>> + vfprintf(stderr, err, params);\n>> + fputc('\\n', stderr);\n>\n> stderr is unbuffered. It's stdout that is linebuffered.\n>\n\nNah, he linebuffered stderr explicitely before.\n"},{"id":"87020","messageId":"20080813072101.GA12628@cuci.nl","threadId":"14966","inReplyTo":"20080813062008.GA4345@blimp.local","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T07:21:01Z","receivedAt":"2008-08-13T07:21:01Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Alex Riesen wrote:\n>Stephen R. van den Berg, Wed, Aug 13, 2008 01:12:10 +0200:\n>> Alex Riesen wrote:\n>> I merely copied the other argument parsing methods, didn't want to\n>> improve this, just functionally equivalent (will do fine here, IMO).\n\n>In case of error max_connections will be 0. How does your code handle\n>it? It is a server side program, which supposed to provide reliable\n>service. In this case, an operator mistake is not even noticable until\n>the first request and even than the clients have to complain first.\n\nWell, same thing that goes wrong when the timeout is set too low,\nof course.\nBut, you have a point, I'll provide a reasonable cap and support\nunlimited as well.\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"And now for something *completely* different!\"\n"},{"id":"87021","messageId":"20080813073613.GB12628@cuci.nl","threadId":"14966","inReplyTo":"7vvdy5pxz6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T07:36:13Z","receivedAt":"2008-08-13T07:36:13Z","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\" writes:\n\n>> Separating it causes two things:\n>> a. The patches to become dependent on each other in the timeline.\n>> b. More (redundant) work, because some parts that need to be rewritten, get\n>> deleted by the following patch(es).\n\n>These are actually desirable properties from reviewability point of view.\n\nOk, I'll look into the splitup.\n\n>>> - I see you have a call to vsyslog, which is the first user of the\n>>> function. How portable is it (the patch coming from you, I know\n>>> Solaris would have it, and recent 4BSD also would, but what about the\n>>> others)?\n\n>> Cygwin has it, Solaris does, Linux does, MacOSX does.\n>> AIX and HPUX don't, perhaps.\n>> I'll see what I can do to avoid it, yet simplify the code.\n\n>That's one of the reasons why I asked you to split it to three patches, so\n>that the syslog change can potentially be independently replaced with a\n>better alternative.\n\nWell, I've already found an alternative, and looks a lot more appealing\nthan the buffer-juggling code of before.\n\n>In any case, it is already late in the rc cycle; I'd like to apply your\n>earlier \"In SysV, signal(SIGCHLD) need to be rearmed\" patch and nothing\n>else for now. The clean-up is very attractive but can be done post 1.6.0.\n\nNo problem. That's the reason I chipped that first patch off; it's a\ndirect bugfix. And BTW, don't mistake me for a \"Solaris guy\". I've\nbeen using Linux almost exclusively since 1991. My SunOS/Solaris knowledge\nhas been fading steadily ever since. It's just that I had to install\ngit-daemon on someone else's Solaris box just recently.\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"And now for something *completely* different!\"\n"},{"id":"87023","messageId":"20080813082342.GC12628@cuci.nl","threadId":"14966","inReplyTo":"20080812225642.GA15265@cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-13T08:23:42Z","receivedAt":"2008-08-13T08:23:42Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Stephen R. van den Berg wrote:\n>Junio C Hamano wrote:\n>> - Taking advantage of poll() getting interrupted by SIGCHLD, so that you\n>> do not have to do anything in the signal handler, is so obvious that I\n>> am actually ashamed of not having to think of it the last time we\n>> touched this code. Is there a poll() that does not return EINTR but\n>> just call the handler and restart after that as if nothing has\n>> happened, I have to wonder...\n\n>Only if the signal is set to SIG_IGN on all systems I worked with since\n>1987.\n\nAnd even if it doesn't get interrupted and restarts, it means that it\nmerely leaves some zombies uncollected until the next new connection to\nthe daemon; nothing to get worried about.\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"And now for something *completely* different!\"\n"},{"id":"87087","messageId":"7v8wv0ogso.fsf@gitster.siamese.dyndns.org","threadId":"14966","inReplyTo":"20080812225642.GA15265@cuci.nl","subject":"Re: [PATCH] git-daemon: Simplify child management and associated logging by","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-13T19:28:55Z","receivedAt":"2008-08-13T19:28:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" writes:\n\n> Junio C Hamano wrote:\n>>\"Stephen R. van den Berg\" writes:\n>>Sorry, but this does too many things in one patch.\n>\n> Yes, I know, got carried away. Then again, the code has a lot of\n> overlapping places (spacewise); I kind of leapt from one place to the\n> next; you fix one thing, and then the next wart stares you in the face.\n> I'll see if I can split it up, if that suits you better.\n>\n>> - Taking advantage of poll() getting interrupted by SIGCHLD, so that you\n>> do not have to do anything in the signal handler, is so obvious that I\n>> am actually ashamed of not having to think of it the last time we\n>> touched this code. Is there a poll() that does not return EINTR but\n>> just call the handler and restart after that as if nothing has\n>> happened, I have to wonder...\n>\n> Only if the signal is set to SIG_IGN on all systems I worked with since\n> 1987.\n\nYeah, rub it in. That's why I said I am actually ashamed that I did not\nnotice that as an obviously much more simpler approach.\n"}]}