{"thread":{"id":"10935","subject":"[PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in","startedAt":"2007-11-19T20:12:54Z","lastAt":"2007-11-21T11:55:22Z","messageCount":5,"participants":["Ping Yin","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"60358","messageId":"1195503174-29387-1-git-send-email-pkufranky@gmail.com","threadId":"10935","inReplyTo":null,"subject":"[PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2007-11-19T20:12:54Z","receivedAt":"2007-11-19T20:12:54Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"When the file descriptor of 'FILE *fp' is assigned to child_process.out\nand then start_command or run_command is run, the standard output of the\nchild process is expected to be outputed to fp.\n\nstart_command will always close fp in this case. However, sometimes fp is\nnot expected to be closed since further IO may be still performmed on fp.\n\nThis patch disables the auto closing behavious of start_command\nand corrects all codes which depend on this kind of behaviour.\n\nFollowing is a case that the auto closing behaviour is not expected.\n\nWhen adding submodule summary feature to builtin-commit, in wt_status_print,\nI want to output the submodule summary between changed and untracked files\nas the following patch shows\n        wt_status_print_changed(s);\n+       wt_status_print_submodule_summary(s);\n        wt_status_print_untracked(s);\n\nAll the three calls will output to s->fp (which points to a file instead of\nstandard output when doing committing). So I don't want s->fp to be closed after\nwt_status_print_submodule_summary(s) which calls run_command.\n\n+static void wt_status_print_submodule_summary(struct wt_status *s)\n+{\n+       struct child_process sm_summary;\n+       memset(&sm_summary, 0, sizeof(sm_summary));\n+       ...\n+       sm_summary.out = fileno(s->fp);\n+       ...\n+       run_command(&sm_summary);\n+}\n\nSigned-off-by: Ping Yin <pkufranky@gmail.com>\n---\n bundle.c      |    1 +\n convert.c     |    1 +\n run-command.c |    4 ----\n upload-pack.c |    3 ++-\n 4 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex e4d60cd..fc253fb 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -336,6 +336,7 @@ int unbundle(struct bundle_header *header, int bundle_fd)\n \tmemset(&ip, 0, sizeof(ip));\n \tip.argv = argv_index_pack;\n \tip.in = bundle_fd;\n+\tip.close_in = 1;\n \tip.no_stdout = 1;\n \tip.git_cmd = 1;\n \tif (run_command(&ip))\ndiff --git a/convert.c b/convert.c\nindex 4df7559..ce7bed0 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -212,6 +212,7 @@ static int filter_buffer(int fd, void *data)\n \tchild_process.argv = argv;\n \tchild_process.in = -1;\n \tchild_process.out = fd;\n+\tchild_process.close_out = 1;\n \n \tif (start_command(&child_process))\n \t\treturn error(\"cannot fork to run external filter %s\", params->cmd);\ndiff --git a/run-command.c b/run-command.c\nindex 476d00c..4e5f58d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -115,13 +115,9 @@ int start_command(struct child_process *cmd)\n \n \tif (need_in)\n \t\tclose(fdin[0]);\n-\telse if (cmd->in)\n-\t\tclose(cmd->in);\n \n \tif (need_out)\n \t\tclose(fdout[1]);\n-\telse if (cmd->out > 1)\n-\t\tclose(cmd->out);\n \n \tif (need_err)\n \t\tclose(fderr[1]);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7e04311..7aeda80 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -163,7 +163,8 @@ static void create_pack_file(void)\n \targv[arg++] = NULL;\n \n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n-\tpack_objects.in = rev_list.out;\t/* start_command closes it */\n+\tpack_objects.in = rev_list.out;\t\n+\tpack_objects.close_in = 1; /* finish_command closes rev_list.out */\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n-- \n1.5.3.5.1878.gb1da0-dirty\n"},{"id":"60411","messageId":"474308A5.8070301@viscovery.net","threadId":"10935","inReplyTo":"1195503174-29387-1-git-send-email-pkufranky@gmail.com","subject":"Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-20T16:17:41Z","receivedAt":"2007-11-20T16:17:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ping Yin schrieb:\n> This patch disables the auto closing behavious of start_command\n> and corrects all codes which depend on this kind of behaviour.\n\nI've thought about this a bit more, and I think that it is better to leave \nthis auto-closing behavior unchanged and change your usage of this feature, \nlike so:\n\n> +static void wt_status_print_submodule_summary(struct wt_status *s)\n> +{\n> +       struct child_process sm_summary;\n> +       memset(&sm_summary, 0, sizeof(sm_summary));\n> +       ...\n> +       sm_summary.out = fileno(s->fp);\n\n\tfflush(s->fp);\n\tsm_summary.out = dup(fileno(s->fp));\t/* run_command closes it */\n\n> +       ...\n> +       run_command(&sm_summary);\n> +}\n\nThis way the change is more local without affecting well-tested other callers.\n\nFurthermore, I don't think that it's correct to just set the .close_in or \n.close_out flags. This will close the fd only in finish_command(), which can \nbe too late: Think again of a writable pipe end that remains open and keeps \nthe reader waiting for input that is not going to happen.\n\n-- Hannes\n"},{"id":"60448","messageId":"46dff0320711201838g5affba6bo21a8c837b0bef681@mail.gmail.com","threadId":"10935","inReplyTo":"474308A5.8070301@viscovery.net","subject":"Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2007-11-21T02:38:33Z","receivedAt":"2007-11-21T02:38:33Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Nov 21, 2007 12:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Ping Yin schrieb:\n> > This patch disables the auto closing behavious of start_command\n> > and corrects all codes which depend on this kind of behaviour.\n>\n> I've thought about this a bit more, and I think that it is better to leave\n> this auto-closing behavior unchanged and change your usage of this feature,\n> like so:\n>\n> > +static void wt_status_print_submodule_summary(struct wt_status *s)\n> > +{\n> > +       struct child_process sm_summary;\n> > +       memset(&sm_summary, 0, sizeof(sm_summary));\n> > +       ...\n> > +       sm_summary.out = fileno(s->fp);\n>\n>         fflush(s->fp);\n>         sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */\n>\n> > +       ...\n> > +       run_command(&sm_summary);\n> > +}\n>\n> This way the change is more local without affecting well-tested other callers.\n>\n\nThis way works, but it is a tricky one, not a natural or graceful one.\n\n> Furthermore, I don't think that it's correct to just set the .close_in or\n> .close_out flags. This will close the fd only in finish_command(), which can\n> be too late: Think again of a writable pipe end that remains open and keeps\n> the reader waiting for input that is not going to happen.\n\nThis may happen. However, i have scanned all the git codes using the\nauto closing behaviour and i don't discover the problem you mentioned.\nSo i think it deserves to correct the misbehaviour after carefully\ntesting. And we can make a clarification for that if necessary.\n\n>\n> -- Hannes\n>\n>\n\n\n\n-- \nPing Yin\n"},{"id":"60484","messageId":"7vmyt8gdzv.fsf@gitster.siamese.dyndns.org","threadId":"10935","inReplyTo":"46dff0320711201838g5affba6bo21a8c837b0bef681@mail.gmail.com","subject":"Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-21T09:11:00Z","receivedAt":"2007-11-21T09:11:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ping Yin\" <pkufranky@gmail.com> writes:\n\n> On Nov 21, 2007 12:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> ...\n>> This way the change is more local without affecting well-tested other callers.\n>\n> This way works, but it is a tricky one, not a natural or graceful one.\n\nI do not know about \"natural\".  That largely would depend on\nwhere one starts thinking about the issues from.\n\nBut I think an API definition that says \"These fds are closed\nafter the call, so if you are going to use them, you can dup()\nthem beforehand\" is equally valid, and I suspect that forgetting\nto dup() is easier to detect than forgetting to close() --- you\nwill notice the former mistake immediately because your read and\nwrite say \"oops, nobody on the other end\" but the latter mistake\nwill result in a hung process.  And for that reason, I think it\ncan be called more \"graceful\".  So ...\n\n>> Furthermore, I don't think that it's correct to just set the .close_in or\n>> .close_out flags. This will close the fd only in finish_command(), which can\n>> be too late: Think again of a writable pipe end that remains open and keeps\n>> the reader waiting for input that is not going to happen.\n>\n> This may happen. However, i have scanned all the git codes using the\n> auto closing behaviour and i don't discover the problem you mentioned.\n> So i think it deserves to correct the misbehaviour after carefully\n> testing. And we can make a clarification for that if necessary.\n\n... I do not necessarily agree that your patch is correcting the\nmisbehaviour.\n"},{"id":"60496","messageId":"46dff0320711210355icdbf634l258cf39c1582e8d4@mail.gmail.com","threadId":"10935","inReplyTo":"7vmyt8gdzv.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2007-11-21T11:55:22Z","receivedAt":"2007-11-21T11:55:22Z","isPatch":true,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Nov 21, 2007 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> But I think an API definition that says \"These fds are closed\n> after the call, so if you are going to use them, you can dup()\n> them beforehand\" is equally valid, and I suspect that forgetting\n> to dup() is easier to detect than forgetting to close() --- you\n> will notice the former mistake immediately because your read and\n> write say \"oops, nobody on the other end\" but the latter mistake\n> will result in a hung process.  And for that reason, I think it\n> can be called more \"graceful\".  So ...\n>\nI don't konw the original API definition and havn't found any API\ndeinition that clarifies the fds will be closed after start_command.\nHowever, when i see child_process.close_in/close_out, i thought\nstart_command will not close the fds.\n\nI never said that start_command must not close fd. At least this\nbehaviour of start_command makes child_process.close_in/close_out no\nsense.\n>\n>\n\n\n\n-- \nPing Yin\n"}]}