{"thread":{"id":"40731","subject":"[PATCH 2/2] strbuf: Correct documentation for strbuf_read_once","startedAt":"2015-11-05T18:17:17Z","lastAt":"2015-11-06T21:41:07Z","messageCount":12,"participants":["Stefan Beller","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"272964","messageId":"1446747439-30349-1-git-send-email-sbeller@google.com","threadId":"40731","inReplyTo":null,"subject":"[PATCH 0/2] Remove non-blocking fds from run-command.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-05T18:17:17Z","receivedAt":"2015-11-05T18:17:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"So as far as I understand, all of the discussion participants (Torsten, Jeff,\nJunio and me) are convinced we don't need the non-blocking feature. So remove it.\n\nI developed it on top of d075d2604c0 (Merge branch 'rs/daemon-plug-child-leak' into sb/submodule-parallel-update)\nbut AFAICT it also applies to sb/submodule-parallel-fetch.\n\nThis will fix compilation in Windows without any platform specific hacks.\n\nThanks,\nStefan\n\nStefan Beller (2):\n run-command: Remove set_nonblocking\n strbuf: Correct documentation for strbuf_read_once\n\n run-command.c | 13 -------------\n strbuf.h | 3 +--\n 2 files changed, 1 insertion(+), 15 deletions(-)\n\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272963","messageId":"1446747439-30349-2-git-send-email-sbeller@google.com","threadId":"40731","inReplyTo":"1446747439-30349-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-05T18:17:18Z","receivedAt":"2015-11-05T18:17:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"strbuf_read_once can also operate on blocking file descriptors if we are\nsure they are ready. The poll (2) command however makes sure this is the\ncase.\n\nReading the manual for poll (2), there may be spurious returns indicating\nreadiness but that is for network sockets only. Pipes should be unaffected.\nBy having this patch, we rely on the correctness of poll to return\nonly pipes ready to read.\n\nThis fixes compilation in Windows.\n\nSigned-off-by: Stefan Beller \n---\n run-command.c | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 0a3c24e..51d078c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1006,17 +1006,6 @@ static void pp_cleanup(struct parallel_processes *pp)\n \tsigchain_pop_common();\n }\n \n-static void set_nonblocking(int fd)\n-{\n-\tint flags = fcntl(fd, F_GETFL);\n-\tif (flags < 0)\n-\t\twarning(\"Could not get file status flags, \"\n-\t\t\t\"output will be degraded\");\n-\telse if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n-\t\twarning(\"Could not set file status flags, \"\n-\t\t\t\"output will be degraded\");\n-}\n-\n /* returns\n * 0 if a new task was started.\n * 1 if no new jobs was started (get_next_task ran out of work, non critical\n@@ -1052,8 +1041,6 @@ static int pp_start_one(struct parallel_processes *pp)\n \t\treturn code ? -1 : 1;\n \t}\n \n-\tset_nonblocking(pp->children[i].process.err);\n-\n \tpp->nr_processes++;\n \tpp->children[i].in_use = 1;\n \tpp->pfd[i].fd = pp->children[i].process.err;\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272962","messageId":"1446747439-30349-3-git-send-email-sbeller@google.com","threadId":"40731","inReplyTo":"1446747439-30349-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/2] strbuf: Correct documentation for strbuf_read_once","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-05T18:17:19Z","receivedAt":"2015-11-05T18:17:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"No need to document the O_NONBLOCK. We will read just once and return.\nIn case the read blocks, this works too.\n\nSigned-off-by: Stefan Beller \n---\n strbuf.h | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex ea69665..7a08da4 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -367,8 +367,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n \n /**\n- * Read from a file descriptor that is marked as O_NONBLOCK without\n- * blocking. Returns the number of new bytes appended to the sb.\n+ * Returns the number of new bytes appended to the sb.\n * Negative return value signals there was an error returned from\n * underlying read(2), in which case the caller should check errno.\n * e.g. errno == EAGAIN when the read may have blocked.\n-- \n2.6.1.247.ge8f2a41.dirty\n"},{"id":"272968","messageId":"xmqqpozo5lqg.fsf@gitster.mtv.corp.google.com","threadId":"40731","inReplyTo":"1446747439-30349-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-05T18:45:11Z","receivedAt":"2015-11-05T18:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> strbuf_read_once can also operate on blocking file descriptors if we are\n> sure they are ready. The poll (2) command however makes sure this is the\n> case.\n>\n> Reading the manual for poll (2), there may be spurious returns indicating\n> readiness but that is for network sockets only. Pipes should be unaffected.\n\nGiven the presence of \"for example\" in that bug section, I wouldn't\nsay \"only\" or \"should be unaffected\".\n\n> By having this patch, we rely on the correctness of poll to return\n> only pipes ready to read.\n\nWe rely on two things. One is for poll to return only pipes that are \nnon-empty. The other is for read from a non-empty pipe not to block.\n\n>\n> This fixes compilation in Windows.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nThanks. Let's apply these fixes on sb/submodule-parallel-fetch,\nmerge the result to 'next' and have people play with it.\n\n> run-command.c | 13 -------------\n> 1 file changed, 13 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 0a3c24e..51d078c 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1006,17 +1006,6 @@ static void pp_cleanup(struct parallel_processes *pp)\n> \tsigchain_pop_common();\n> }\n> \n> -static void set_nonblocking(int fd)\n> -{\n> -\tint flags = fcntl(fd, F_GETFL);\n> -\tif (flags < 0)\n> -\t\twarning(\"Could not get file status flags, \"\n> -\t\t\t\"output will be degraded\");\n> -\telse if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n> -\t\twarning(\"Could not set file status flags, \"\n> -\t\t\t\"output will be degraded\");\n> -}\n> -\n> /* returns\n> * 0 if a new task was started.\n> * 1 if no new jobs was started (get_next_task ran out of work, non critical\n> @@ -1052,8 +1041,6 @@ static int pp_start_one(struct parallel_processes *pp)\n> \t\treturn code ? -1 : 1;\n> \t}\n> \n> -\tset_nonblocking(pp->children[i].process.err);\n> -\n> \tpp->nr_processes++;\n> \tpp->children[i].in_use = 1;\n> \tpp->pfd[i].fd = pp->children[i].process.err;\n"},{"id":"272969","messageId":"CAGZ79kZgEtKoYqxa8+wj0PCJedW140CQAPX6XwEYib1W3fPhXw@mail.gmail.com","threadId":"40731","inReplyTo":"xmqqpozo5lqg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-05T19:22:29Z","receivedAt":"2015-11-05T19:22:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 5, 2015 at 10:45 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> strbuf_read_once can also operate on blocking file descriptors if we are\n>> sure they are ready. The poll (2) command however makes sure this is the\n>> case.\n>>\n>> Reading the manual for poll (2), there may be spurious returns indicating\n>> readiness but that is for network sockets only. Pipes should be unaffected.\n>\n> Given the presence of \"for example\" in that bug section, I wouldn't\n> say \"only\" or \"should be unaffected\".\n\nReading the documentation we are in agreement, that we expect\nno spurious returns, no?\n\n>\n>> By having this patch, we rely on the correctness of poll to return\n>> only pipes ready to read.\n>\n> We rely on two things. One is for poll to return only pipes that are\n> non-empty. The other is for read from a non-empty pipe not to block.\n\nThat's what I meant with 'pipe being ready'.\n\n>\n>>\n>> This fixes compilation in Windows.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>\n> Thanks. Let's apply these fixes on sb/submodule-parallel-fetch,\n> merge the result to 'next' and have people play with it.\n\nMaybe the commit message was weakly crafted. Do you want me to resend?\n\n>\n>> run-command.c | 13 -------------\n>> 1 file changed, 13 deletions(-)\n>>\n>> diff --git a/run-command.c b/run-command.c\n>> index 0a3c24e..51d078c 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -1006,17 +1006,6 @@ static void pp_cleanup(struct parallel_processes *pp)\n>> sigchain_pop_common();\n>> }\n>>\n>> -static void set_nonblocking(int fd)\n>> -{\n>> - int flags = fcntl(fd, F_GETFL);\n>> - if (flags < 0)\n>> - warning(\"Could not get file status flags, \"\n>> - \"output will be degraded\");\n>> - else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n>> - warning(\"Could not set file status flags, \"\n>> - \"output will be degraded\");\n>> -}\n>> -\n>> /* returns\n>> * 0 if a new task was started.\n>> * 1 if no new jobs was started (get_next_task ran out of work, non critical\n>> @@ -1052,8 +1041,6 @@ static int pp_start_one(struct parallel_processes *pp)\n>> return code ? -1 : 1;\n>> }\n>>\n>> - set_nonblocking(pp->children[i].process.err);\n>> -\n>> pp->nr_processes++;\n>> pp->children[i].in_use = 1;\n>> pp->pfd[i].fd = pp->children[i].process.err;\n"},{"id":"272970","messageId":"xmqqio5g5jc3.fsf@gitster.mtv.corp.google.com","threadId":"40731","inReplyTo":"CAGZ79kZgEtKoYqxa8+wj0PCJedW140CQAPX6XwEYib1W3fPhXw@mail.gmail.com","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-05T19:37:00Z","receivedAt":"2015-11-05T19:37:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Nov 5, 2015 at 10:45 AM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> strbuf_read_once can also operate on blocking file descriptors if we are\n>>> sure they are ready. The poll (2) command however makes sure this is the\n>>> case.\n>>>\n>>> Reading the manual for poll (2), there may be spurious returns indicating\n>>> readiness but that is for network sockets only. Pipes should be unaffected.\n>>\n>> Given the presence of \"for example\" in that bug section, I wouldn't\n>> say \"only\" or \"should be unaffected\".\n>\n> Reading the documentation we are in agreement, that we expect\n> no spurious returns, no?\n\nGiven the presence of \"for example\" in that bug section, I wouldn't\nsay \"only\" or \"should be unaffected\". I cannot say \"we expect no\nspurious returns\".\n\n>> Thanks. Let's apply these fixes on sb/submodule-parallel-fetch,\n>> merge the result to 'next' and have people play with it.\n>\n> Maybe the commit message was weakly crafted. Do you want me to resend?\n\nI somehow feel that it is prudent to let this cook just above 'next'\nfor a few days (not just for the log message but to verify the\nstrategy and wait for others to come up with even better ideas), but\nthen I'll be offline starting next week, so I expect that merging\nthe final version to 'next' will be done by our interim maintainer,\nwhich means we still have time to polish ;-)\n\nHere is what I queued for now.\n\n-- >8 --\nFrom: Stefan Beller \nDate: Thu, 5 Nov 2015 10:17:18 -0800\nSubject: [PATCH] run-command: remove set_nonblocking()\n\nstrbuf_read_once can also operate on blocking file descriptors if we\nare sure they are ready. And the poll(2) we call before calling\nthis ensures that this is the case.\n\nReading the manual for poll(2), there may be spurious returns\nindicating readiness but that is for network sockets only and pipes\nshould be unaffected.\n\nWith this change, we rely on\n\n - poll(2) returns only non-empty pipes; and\n - read(2) on a non-empty pipe does not block.\n\nThis should fix compilation on Windows.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n run-command.c | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1fbd286..07424e9 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -996,17 +996,6 @@ static void pp_cleanup(struct parallel_processes *pp)\n \tsigchain_pop_common();\n }\n \n-static void set_nonblocking(int fd)\n-{\n-\tint flags = fcntl(fd, F_GETFL);\n-\tif (flags < 0)\n-\t\twarning(\"Could not get file status flags, \"\n-\t\t\t\"output will be degraded\");\n-\telse if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n-\t\twarning(\"Could not set file status flags, \"\n-\t\t\t\"output will be degraded\");\n-}\n-\n /* returns\n * 0 if a new task was started.\n * 1 if no new jobs was started (get_next_task ran out of work, non critical\n@@ -1042,8 +1031,6 @@ static int pp_start_one(struct parallel_processes *pp)\n \t\treturn code ? -1 : 1;\n \t}\n \n-\tset_nonblocking(pp->children[i].process.err);\n-\n \tpp->nr_processes++;\n \tpp->children[i].in_use = 1;\n \tpp->pfd[i].fd = pp->children[i].process.err;\n-- \n2.6.2-539-g1c5cd50\n"},{"id":"272971","messageId":"563BBBBC.7070807@kdbg.org","threadId":"40731","inReplyTo":"1446747439-30349-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-05T20:27:40Z","receivedAt":"2015-11-05T20:27:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.11.2015 um 19:17 schrieb Stefan Beller:\n> strbuf_read_once can also operate on blocking file descriptors if we are\n> sure they are ready. The poll (2) command however makes sure this is the\n> case.\n> \n> Reading the manual for poll (2), there may be spurious returns indicating\n> readiness but that is for network sockets only. Pipes should be unaffected.\n> By having this patch, we rely on the correctness of poll to return\n> only pipes ready to read.\n> \n> This fixes compilation in Windows.\n\nIt certainly does (but I haven't tested, yet). But parallel processes\nwill not work because we do not have a sufficiently complete waitpid\nemulation, yet. (waitpid(-1, ...) is not implemented.)\n\nHowever, I think that the infrastructure can be simplified even further\nto a level that we do not need additional emulation on Windows.\n\nFirst let me say that I find it very questionable that the callbacks\nreceive a struct child_process. This is an implementation detail. It is\nalso an implementation detail that stderr of the children is read and\nbuffered, and that the child's stdout is redirected to stderr. It\nshould not be the task of the get_next_task callback to set the members\nno_stdin, stdout_to_stderr, and err of struct child_process.\n\nIf you move that initialization to pp_start_one, you will notice rather\nsooner than later that the readable end of the file descriptor is never\nclosed!\n\nWhich makes me think: Other users of start_command/finish_command work\nsuch that they\n\n1. request a pipe by setting .out = -1\n2. start_command\n3. read from .out until EOF\n4. close .out\n5. wait for the process with finish_command\n\nBut the parallel_process infrastructure does not follow this pattern.\nIt\n\n1. requests a pipe by setting .err = -1\n2. start_command\n3. read from .err\n4. wait for the process with waitpid\n\n(and forgets to close .err). EOF is not in the picture (but that is\nnot essential).\n\nI suggest to change this such that we read from the children until EOF,\nmark them to be at their end of life, and then wait for them using\nfinish_command (assuming that a process that closes stdout and stderr\nwill die very soon if it is not already dead).\n\nHere is a prototype patch. Feel free to pick it up. It marks a process\nwhose EOF we have found by setting .err to -1. It's probably better to\nextend the meaning of the in_use indicator for this purpose. This seems\nto work on Linux with test-run-command with sub-processes that produce\n100k output each:\n\n./test-run-command run-command-parallel 5 sh -c \"printf \\\"%0100000d\\n\\\" 999\"\n\nalthough error handling would require some polishing according to\nt0061-run-command.\n\ndiff --git a/run-command.c b/run-command.c\nindex 51d078c..3e42299 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,\n \tfor (i = 0; i < n; i++) {\n \t\tstrbuf_init(&pp->children[i].err, 0);\n \t\tchild_process_init(&pp->children[i].process);\n-\t\tpp->pfd[i].events = POLLIN;\n+\t\tpp->pfd[i].events = POLLIN|POLLHUP;\n \t\tpp->pfd[i].fd = -1;\n \t}\n \tsigchain_push_common(handle_children_on_signal);\n@@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n \t/* Buffer output from all pipes. */\n \tfor (i = 0; i < pp->max_processes; i++) {\n \t\tif (pp->children[i].in_use &&\n-\t\t pp->pfd[i].revents & POLLIN)\n-\t\t\tif (strbuf_read_once(&pp->children[i].err,\n-\t\t\t\t\t pp->children[i].process.err, 0) < 0)\n+\t\t pp->pfd[i].revents & (POLLIN|POLLHUP)) {\n+\t\t\tint n = strbuf_read_once(&pp->children[i].err,\n+\t\t\t\t\t pp->children[i].process.err, 0);\n+\t\t\tif (n == 0) {\n+\t\t\t\tclose(pp->children[i].process.err);\n+\t\t\t\tpp->children[i].process.err = -1;\n+\t\t\t} else if (n < 0) {\n \t\t\t\tif (errno != EAGAIN)\n \t\t\t\t\tdie_errno(\"read\");\n+\t\t\t}\n+\t\t}\n \t}\n }\n \n@@ -1082,59 +1088,20 @@ static void pp_output(struct parallel_processes *pp)\n static int pp_collect_finished(struct parallel_processes *pp)\n {\n \tint i = 0;\n-\tpid_t pid;\n-\tint wait_status, code;\n+\tint code;\n \tint n = pp->max_processes;\n \tint result = 0;\n \n \twhile (pp->nr_processes > 0) {\n-\t\tpid = waitpid(-1, &wait_status, WNOHANG);\n-\t\tif (pid == 0)\n-\t\t\tbreak;\n-\n-\t\tif (pid < 0)\n-\t\t\tdie_errno(\"wait\");\n-\n \t\tfor (i = 0; i < pp->max_processes; i++)\n \t\t\tif (pp->children[i].in_use &&\n-\t\t\t pid == pp->children[i].process.pid)\n+\t\t\t pp->children[i].process.err == -1)\n \t\t\t\tbreak;\n+\n \t\tif (i == pp->max_processes)\n-\t\t\tdie(\"BUG: found a child process we were not aware of\");\n-\n-\t\tif (strbuf_read(&pp->children[i].err,\n-\t\t\t\tpp->children[i].process.err, 0) < 0)\n-\t\t\tdie_errno(\"strbuf_read\");\n-\n-\t\tif (WIFSIGNALED(wait_status)) {\n-\t\t\tcode = WTERMSIG(wait_status);\n-\t\t\tif (!pp->shutdown &&\n-\t\t\t code != SIGINT && code != SIGQUIT)\n-\t\t\t\tstrbuf_addf(&pp->children[i].err,\n-\t\t\t\t\t \"%s died of signal %d\",\n-\t\t\t\t\t pp->children[i].process.argv[0],\n-\t\t\t\t\t code);\n-\t\t\t/*\n-\t\t\t * This return value is chosen so that code & 0xff\n-\t\t\t * mimics the exit code that a POSIX shell would report for\n-\t\t\t * a program that died from this signal.\n-\t\t\t */\n-\t\t\tcode += 128;\n-\t\t} else if (WIFEXITED(wait_status)) {\n-\t\t\tcode = WEXITSTATUS(wait_status);\n-\t\t\t/*\n-\t\t\t * Convert special exit code when execvp failed.\n-\t\t\t */\n-\t\t\tif (code == 127) {\n-\t\t\t\tcode = -1;\n-\t\t\t\terrno = ENOENT;\n-\t\t\t}\n-\t\t} else {\n-\t\t\tstrbuf_addf(&pp->children[i].err,\n-\t\t\t\t \"waitpid is confused (%s)\",\n-\t\t\t\t pp->children[i].process.argv[0]);\n-\t\t\tcode = -1;\n-\t\t}\n+\t\t\tbreak;\n+\n+\t\tcode = finish_command(&pp->children[i].process);\n \n \t\tif (pp->task_finished(code, &pp->children[i].process,\n \t\t\t\t &pp->children[i].err, pp->data,\n@@ -1144,7 +1111,6 @@ static int pp_collect_finished(struct parallel_processes *pp)\n \t\tpp->nr_processes--;\n \t\tpp->children[i].in_use = 0;\n \t\tpp->pfd[i].fd = -1;\n-\t\tchild_process_clear(&pp->children[i].process);\n \t\tchild_process_init(&pp->children[i].process);\n \n \t\tif (i != pp->output_owner) {\n"},{"id":"272973","messageId":"xmqqa8qs5fxi.fsf@gitster.mtv.corp.google.com","threadId":"40731","inReplyTo":"563BBBBC.7070807@kdbg.org","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-05T20:50:33Z","receivedAt":"2015-11-05T20:50:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 05.11.2015 um 19:17 schrieb Stefan Beller:\n>> strbuf_read_once can also operate on blocking file descriptors if we are\n>> sure they are ready. The poll (2) command however makes sure this is the\n>> case.\n>> \n>> Reading the manual for poll (2), there may be spurious returns indicating\n>> readiness but that is for network sockets only. Pipes should be unaffected.\n>> By having this patch, we rely on the correctness of poll to return\n>> only pipes ready to read.\n>> \n>> This fixes compilation in Windows.\n>\n> It certainly does (but I haven't tested, yet). But parallel processes\n> will not work because we do not have a sufficiently complete waitpid\n> emulation, yet. (waitpid(-1, ...) is not implemented.)\n>\n> However, I think that the infrastructure can be simplified even further\n> to a level that we do not need additional emulation on Windows.\n\n;-)\n\nThis is why I love this list (and in general not rushing any change\ntoo early to 'next').\n\n> Which makes me think: Other users of start_command/finish_command work\n> such that they\n>\n> 1. request a pipe by setting .out = -1\n> 2. start_command\n> 3. read from .out until EOF\n> 4. close .out\n> 5. wait for the process with finish_command\n>\n> But the parallel_process infrastructure does not follow this pattern.\n> It\n>\n> 1. requests a pipe by setting .err = -1\n> 2. start_command\n> 3. read from .err\n> 4. wait for the process with waitpid\n>\n> (and forgets to close .err). EOF is not in the picture (but that is\n> not essential).\n\nUnrelated tangent. daemon is another one that uses start_command()\nbut does not use finish_command().\n\n> I suggest to change this such that we read from the children until EOF,\n> mark them to be at their end of life, and then wait for them using\n> finish_command (assuming that a process that closes stdout and stderr\n> will die very soon if it is not already dead).\n\nHmm, interesting. This does match the normal \"spawn, interact and\nwait\" cycle for a single process much better.\n"},{"id":"272976","messageId":"CAGZ79kZs8HQegNy=L5nGMUGJ98s-xgce1Ncrxpxe_ReaJXMjKw@mail.gmail.com","threadId":"40731","inReplyTo":"563BBBBC.7070807@kdbg.org","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-05T22:20:57Z","receivedAt":"2015-11-05T22:20:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt wrote:\n\n> diff --git a/run-command.c b/run-command.c\n> index 51d078c..3e42299 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,\n> for (i = 0; i < n; i++) {\n> strbuf_init(&pp->children[i].err, 0);\n> child_process_init(&pp->children[i].process);\n> - pp->pfd[i].events = POLLIN;\n> + pp->pfd[i].events = POLLIN|POLLHUP;\n> pp->pfd[i].fd = -1;\n> }\n> sigchain_push_common(handle_children_on_signal);\n> @@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n> /* Buffer output from all pipes. */\n> for (i = 0; i < pp->max_processes; i++) {\n> if (pp->children[i].in_use &&\n> - pp->pfd[i].revents & POLLIN)\n> - if (strbuf_read_once(&pp->children[i].err,\n> - pp->children[i].process.err, 0) < 0)\n> + pp->pfd[i].revents & (POLLIN|POLLHUP)) {\n> + int n = strbuf_read_once(&pp->children[i].err,\n> + pp->children[i].process.err, 0);\n> + if (n == 0) {\n> + close(pp->children[i].process.err);\n> + pp->children[i].process.err = -1;\n\nSo you set .err to -1 to signal the process has ended here...\n\n> -\n> for (i = 0; i < pp->max_processes; i++)\n> if (pp->children[i].in_use &&\n> - pid == pp->children[i].process.pid)\n> + pp->children[i].process.err == -1)\n> break;\n\nto make a decision here if we want to finish_command on it.\n\n\n> + code = finish_command(&pp->children[i].process);\n\n> - child_process_clear(&pp->children[i].process);\n\nbut .err stays stays -1 here for the next iteration?\nWe would need to reset it to 0 again.\n\nSo .err is\n 0 when the slot is not in use\n -1 when the child has finished awaiting termination\n >0 when the child is living a happy life.\n"},{"id":"272993","messageId":"563C3FDF.9050304@kdbg.org","threadId":"40731","inReplyTo":"CAGZ79kZs8HQegNy=L5nGMUGJ98s-xgce1Ncrxpxe_ReaJXMjKw@mail.gmail.com","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-06T05:51:27Z","receivedAt":"2015-11-06T05:51:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.11.2015 um 23:20 schrieb Stefan Beller:\n> On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt wrote:\n>\n>> diff --git a/run-command.c b/run-command.c\n>> index 51d078c..3e42299 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,\n>> for (i = 0; i < n; i++) {\n>> strbuf_init(&pp->children[i].err, 0);\n>> child_process_init(&pp->children[i].process);\n>> - pp->pfd[i].events = POLLIN;\n>> + pp->pfd[i].events = POLLIN|POLLHUP;\n>> pp->pfd[i].fd = -1;\n>> }\n>> sigchain_push_common(handle_children_on_signal);\n>> @@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout)\n>> /* Buffer output from all pipes. */\n>> for (i = 0; i < pp->max_processes; i++) {\n>> if (pp->children[i].in_use &&\n>> - pp->pfd[i].revents & POLLIN)\n>> - if (strbuf_read_once(&pp->children[i].err,\n>> - pp->children[i].process.err, 0) < 0)\n>> + pp->pfd[i].revents & (POLLIN|POLLHUP)) {\n>> + int n = strbuf_read_once(&pp->children[i].err,\n>> + pp->children[i].process.err, 0);\n>> + if (n == 0) {\n>> + close(pp->children[i].process.err);\n>> + pp->children[i].process.err = -1;\n>\n> So you set .err to -1 to signal the process has ended here...\n>\n>> -\n>> for (i = 0; i < pp->max_processes; i++)\n>> if (pp->children[i].in_use &&\n>> - pid == pp->children[i].process.pid)\n>> + pp->children[i].process.err == -1)\n>> break;\n>\n> to make a decision here if we want to finish_command on it.\n\nCorrect.\n\n>> + code = finish_command(&pp->children[i].process);\n>\n>> - child_process_clear(&pp->children[i].process);\n>\n> but .err stays stays -1 here for the next iteration?\n> We would need to reset it to 0 again.\n\nNo. In the next round, we need -1 to request a pipe. get_next_task \ncallback sets it to -1 as well (and I think it is wrong that the \ncallback does it; pp_start_one should do that).\n\n> So .err is\n> 0 when the slot is not in use\n> -1 when the child has finished awaiting termination\n> >0 when the child is living a happy life.\n\nBut, as I said, .err is not the right place to mark dying processes (it \nwas just the simplest way to demonstrate the concept in this patch). \nBetter extend .in_use to a tri-state indicator.\n\n-- Hannes\n"},{"id":"273031","messageId":"CAGZ79kbNpwhOF2eveux+yXibrYx+_S6KjvqLV7yEfAe5Sdg4uA@mail.gmail.com","threadId":"40731","inReplyTo":"563BBBBC.7070807@kdbg.org","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-06T19:00:53Z","receivedAt":"2015-11-06T19:00:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt wrote:\n> However, I think that the infrastructure can be simplified even further\n> to a level that we do not need additional emulation on Windows.\n>\n> First let me say that I find it very questionable that the callbacks\n> receive a struct child_process.\n\nok, I try to clean that up as well. However I made this choice as we\nneed to set the working dir as well as the environment variables.\nI guess these will go into an extra argument then\n\n\n> Here is a prototype patch. Feel free to pick it up. It marks a process\n> whose EOF we have found by setting .err to -1. It's probably better to\n> extend the meaning of the in_use indicator for this purpose.\n\nThanks for the proposal, I'll take that and make in_use a tristate for now\n(an enum consisting of FREE, WORKING, WAIT_CLEANUP)\n\nThanks,\nStefan\n"},{"id":"273032","messageId":"563D1E73.8080805@kdbg.org","threadId":"40731","inReplyTo":"CAGZ79kbNpwhOF2eveux+yXibrYx+_S6KjvqLV7yEfAe5Sdg4uA@mail.gmail.com","subject":"Re: [PATCH 1/2] run-command: Remove set_nonblocking","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-11-06T21:41:07Z","receivedAt":"2015-11-06T21:41:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.11.2015 um 20:00 schrieb Stefan Beller:\n> On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt wrote:\n>> Here is a prototype patch. Feel free to pick it up. It marks a process\n>> whose EOF we have found by setting .err to -1. It's probably better to\n>> extend the meaning of the in_use indicator for this purpose.\n>\n> Thanks for the proposal, I'll take that and make in_use a tristate for now\n> (an enum consisting of FREE, WORKING, WAIT_CLEANUP)\n\nI'd like to report that the prototype patch works on Windows. I tested \nit lightly using test-run-command with commands producing output from \naround 100 bytes to 10MB. So, I'm confident that this is the right approach.\n\nThank you for keeping the ball rolling!\n\n-- Hannes\n"}]}