threads / patch / 10935

patchFix start_command closing cmd->out/in regardless of cmd->close_out/in

Subject: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

## tl;dr

5 messages between Nov 19, 2007 and Nov 21, 2007. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Ping Yin· Nov 19, 2007, 20:12 UTC · lore

When the file descriptor of 'FILE *fp' is assigned to child_process.out and then start_command or run_command is run, the standard output of the child process is expected to be outputed to fp.

start_command will always close fp in this case. However, sometimes fp is not expected to be closed since further IO may be still performmed on fp.

This patch disables the auto closing behavious of start_command and corrects all codes which depend on this kind of behaviour.

Following is a case that the auto closing behaviour is not expected.
When adding submodule summary feature to builtin-commit, in wt_status_print,
I want to output the submodule summary between changed and untracked files
as the following patch shows
        wt_status_print_changed(s);
+       wt_status_print_submodule_summary(s);
        wt_status_print_untracked(s);

All the three calls will output to s->fp (which points to a file instead of standard output when doing committing). So I don't want s->fp to be closed after wt_status_print_submodule_summary(s) which calls run_command.

+static void wt_status_print_submodule_summary(struct wt_status *s)
+{
+       struct child_process sm_summary;
+       memset(&sm_summary, 0, sizeof(sm_summary));
+       ...
+       sm_summary.out = fileno(s->fp);
+       ...
+       run_command(&sm_summary);
+}
Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 bundle.c      |    1 +
 convert.c     |    1 +
 run-command.c |    4 ----
 upload-pack.c |    3 ++-
 4 files changed, 4 insertions(+), 5 deletions(-)
Show changes to 4 files +4 −5

bundle.c, convert.c, run-command.c, upload-pack.c

diff --git a/bundle.c b/bundle.c
index e4d60cd..fc253fb 100644
--- a/bundle.c
+++ b/bundle.c
@@ -336,6 +336,7 @@ int unbundle(struct bundle_header *header, int bundle_fd)
 	memset(&ip, 0, sizeof(ip));
 	ip.argv = argv_index_pack;
 	ip.in = bundle_fd;
+	ip.close_in = 1;
 	ip.no_stdout = 1;
 	ip.git_cmd = 1;
 	if (run_command(&ip))
diff --git a/convert.c b/convert.c
index 4df7559..ce7bed0 100644
--- a/convert.c
+++ b/convert.c
@@ -212,6 +212,7 @@ static int filter_buffer(int fd, void *data)
 	child_process.argv = argv;
 	child_process.in = -1;
 	child_process.out = fd;
+	child_process.close_out = 1;
 
 	if (start_command(&child_process))
 		return error("cannot fork to run external filter %s", params->cmd);
diff --git a/run-command.c b/run-command.c
index 476d00c..4e5f58d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -115,13 +115,9 @@ int start_command(struct child_process *cmd)
 
 	if (need_in)
 		close(fdin[0]);
-	else if (cmd->in)
-		close(cmd->in);
 
 	if (need_out)
 		close(fdout[1]);
-	else if (cmd->out > 1)
-		close(cmd->out);
 
 	if (need_err)
 		close(fderr[1]);
diff --git a/upload-pack.c b/upload-pack.c
index 7e04311..7aeda80 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -163,7 +163,8 @@ static void create_pack_file(void)
 	argv[arg++] = NULL;
 
 	memset(&pack_objects, 0, sizeof(pack_objects));
-	pack_objects.in = rev_list.out;	/* start_command closes it */
+	pack_objects.in = rev_list.out;	
+	pack_objects.close_in = 1; /* finish_command closes rev_list.out */
 	pack_objects.out = -1;
 	pack_objects.err = -1;
 	pack_objects.git_cmd = 1;

-- 
1.5.3.5.1878.gb1da0-dirty
Johannes Sixt· Nov 20, 2007, 16:17 UTC · re: Ping Yin · lore

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

Ping Yin schrieb:
> This patch disables the auto closing behavious of start_command
> and corrects all codes which depend on this kind of behaviour.

I've thought about this a bit more, and I think that it is better to leave this auto-closing behavior unchanged and change your usage of this feature, like so:

Show 6 quoted lines
> +static void wt_status_print_submodule_summary(struct wt_status *s)
> +{
> +       struct child_process sm_summary;
> +       memset(&sm_summary, 0, sizeof(sm_summary));
> +       ...
> +       sm_summary.out = fileno(s->fp);
	fflush(s->fp);
	sm_summary.out = dup(fileno(s->fp));	/* run_command closes it */
> +       ...
> +       run_command(&sm_summary);
> +}
This way the change is more local without affecting well-tested other callers.

Furthermore, I don't think that it's correct to just set the .close_in or .close_out flags. This will close the fd only in finish_command(), which can be too late: Think again of a writable pipe end that remains open and keeps the reader waiting for input that is not going to happen.

-- Hannes
Ping Yin· Nov 21, 2007, 02:38 UTC · re: Johannes Sixt · lore

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

On Nov 21, 2007 12:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 24 quoted lines
> Ping Yin schrieb:
> > This patch disables the auto closing behavious of start_command
> > and corrects all codes which depend on this kind of behaviour.
>
> I've thought about this a bit more, and I think that it is better to leave
> this auto-closing behavior unchanged and change your usage of this feature,
> like so:
>
> > +static void wt_status_print_submodule_summary(struct wt_status *s)
> > +{
> > +       struct child_process sm_summary;
> > +       memset(&sm_summary, 0, sizeof(sm_summary));
> > +       ...
> > +       sm_summary.out = fileno(s->fp);
>
>         fflush(s->fp);
>         sm_summary.out = dup(fileno(s->fp));    /* run_command closes it */
>
> > +       ...
> > +       run_command(&sm_summary);
> > +}
>
> This way the change is more local without affecting well-tested other callers.
>
This way works, but it is a tricky one, not a natural or graceful one.
> Furthermore, I don't think that it's correct to just set the .close_in or
> .close_out flags. This will close the fd only in finish_command(), which can
> be too late: Think again of a writable pipe end that remains open and keeps
> the reader waiting for input that is not going to happen.

This may happen. However, i have scanned all the git codes using the auto closing behaviour and i don't discover the problem you mentioned. So i think it deserves to correct the misbehaviour after carefully testing. And we can make a clarification for that if necessary.

>
> -- Hannes
>
>
-- 
Ping Yin
Junio C Hamano· Nov 21, 2007, 09:11 UTC · re: Ping Yin · lore

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

"Ping Yin" <pkufranky@gmail.com> writes:
Show 5 quoted lines
> On Nov 21, 2007 12:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> ...
>> This way the change is more local without affecting well-tested other callers.
>
> This way works, but it is a tricky one, not a natural or graceful one.

I do not know about "natural". That largely would depend on where one starts thinking about the issues from.

But I think an API definition that says "These fds are closed after the call, so if you are going to use them, you can dup() them beforehand" is equally valid, and I suspect that forgetting to dup() is easier to detect than forgetting to close() --- you will notice the former mistake immediately because your read and write say "oops, nobody on the other end" but the latter mistake will result in a hung process. And for that reason, I think it can be called more "graceful". So ...

Show 9 quoted lines
>> Furthermore, I don't think that it's correct to just set the .close_in or
>> .close_out flags. This will close the fd only in finish_command(), which can
>> be too late: Think again of a writable pipe end that remains open and keeps
>> the reader waiting for input that is not going to happen.
>
> This may happen. However, i have scanned all the git codes using the
> auto closing behaviour and i don't discover the problem you mentioned.
> So i think it deserves to correct the misbehaviour after carefully
> testing. And we can make a clarification for that if necessary.

... I do not necessarily agree that your patch is correcting the misbehaviour.

Ping Yin· Nov 21, 2007, 11:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix start_command closing cmd->out/in regardless of cmd->close_out/in

On Nov 21, 2007 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> But I think an API definition that says "These fds are closed
> after the call, so if you are going to use them, you can dup()
> them beforehand" is equally valid, and I suspect that forgetting
> to dup() is easier to detect than forgetting to close() --- you
> will notice the former mistake immediately because your read and
> write say "oops, nobody on the other end" but the latter mistake
> will result in a hung process.  And for that reason, I think it
> can be called more "graceful".  So ...
>

I don't konw the original API definition and havn't found any API deinition that clarifies the fds will be closed after start_command. However, when i see child_process.close_in/close_out, i thought start_command will not close the fds.

I never said that start_command must not close fd. At least this behaviour of start_command makes child_process.close_in/close_out no sense.

>
>
-- 
Ping Yin

← back to recent threads