start_command(), .in/.out/.err = -1: Callers must close the file descriptor

Johannes Sixt · gitster · commit e72ae28895b2 · 2008-02-23T11:59:44.000-08:00
By setting .in, .out, or .err members of struct child_process to -1, the
callers of start_command() can request that a pipe is allocated that talks
to the child process and one end is returned by replacing -1 with the
file descriptor.

Previously, a flag was set (for .in and .out, but not .err) to signal
finish_command() to close the pipe end that start_command() had handed out,
so it was optional for callers to close the pipe, and many already do so.
Now we make it mandatory to close the pipe.

Signed-off-by: Johannes Sixt &lt;johannes.sixt@telecom.at&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
@@ -538,8 +538,10 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	cmd.git_cmd = 1;
 	if (start_command(&cmd))
 		die("fetch-pack: unable to fork off %s", argv[0]);
-	if (do_keep && pack_lockfile)
+	if (do_keep && pack_lockfile) {
 		*pack_lockfile = index_pack_lockfile(cmd.out);
+		close(cmd.out);
+	}
 
 	if (finish_command(&cmd))
 		die("%s failed", argv[0]);
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
@@ -71,6 +71,7 @@ static int pack_objects(int fd, struct ref *refs)
 		refs = refs->next;
 	}
 
+	close(po.in);
 	if (finish_command(&po))
 		return error("pack-objects died with strange error");
 	return 0;
diff --git a/builtin-tag.c b/builtin-tag.c
@@ -226,12 +226,13 @@ static int do_sign(struct strbuf *buffer)
 
 	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
 		close(gpg.in);
+		close(gpg.out);
 		finish_command(&gpg);
 		return error("gpg did not accept the tag data");
 	}
 	close(gpg.in);
-	gpg.close_in = 0;
 	len = strbuf_read(buffer, gpg.out, 1024);
+	close(gpg.out);
 
 	if (finish_command(&gpg) || !len || len < 0)
 		return error("gpg failed to sign the tag");
diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
@@ -52,7 +52,6 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 
 	write_in_full(gpg.in, buf, len);
 	close(gpg.in);
-	gpg.close_in = 0;
 	ret = finish_command(&gpg);
 
 	unlink(path);
diff --git a/bundle.c b/bundle.c
@@ -333,6 +333,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 		write_or_die(rls.in, sha1_to_hex(object->sha1), 40);
 		write_or_die(rls.in, "\n", 1);
 	}
+	close(rls.in);
 	if (finish_command(&rls))
 		return error ("pack-objects died");
 
diff --git a/receive-pack.c b/receive-pack.c
@@ -132,6 +132,7 @@ static int run_hook(const char *hook_name)
 				break;
 		}
 	}
+	close(proc.in);
 	return hook_status(finish_command(&proc), hook_name);
 }
 
@@ -414,6 +415,7 @@ static const char *unpack(void)
 		if (start_command(&ip))
 			return "index-pack fork failed";
 		pack_lockfile = index_pack_lockfile(ip.out);
+		close(ip.out);
 		status = finish_command(&ip);
 		if (!status) {
 			reprepare_packed_git();
diff --git a/run-command.c b/run-command.c
@@ -25,7 +25,6 @@ int start_command(struct child_process *cmd)
 		if (pipe(fdin) < 0)
 			return -ERR_RUN_COMMAND_PIPE;
 		cmd->in = fdin[1];
-		cmd->close_in = 1;
 	}
 
 	need_out = !cmd->no_stdout
@@ -38,7 +37,6 @@ int start_command(struct child_process *cmd)
 			return -ERR_RUN_COMMAND_PIPE;
 		}
 		cmd->out = fdout[0];
-		cmd->close_out = 1;
 	}
 
 	need_err = !cmd->no_stderr && cmd->err < 0;
@@ -157,10 +155,6 @@ static int wait_or_whine(pid_t pid)
 
 int finish_command(struct child_process *cmd)
 {
-	if (cmd->close_in)
-		close(cmd->in);
-	if (cmd->close_out)
-		close(cmd->out);
 	return wait_or_whine(cmd->pid);
 }
 
diff --git a/run-command.h b/run-command.h
@@ -19,8 +19,6 @@ struct child_process {
 	int err;
 	const char *dir;
 	const char *const *env;
-	unsigned close_in:1;
-	unsigned close_out:1;
 	unsigned no_stdin:1;
 	unsigned no_stdout:1;
 	unsigned no_stderr:1;

Original file line number	Diff line number	Diff line change
`@@ -71,6 +71,7 @@ static int pack_objects(int fd, struct ref *refs)`
`71`	`71`	`refs = refs->next;`
`72`	`72`	`}`
`73`	`73`
	`74`	`+ close(po.in);`
`74`	`75`	`if (finish_command(&po))`
`75`	`76`	`return error("pack-objects died with strange error");`
`76`	`77`	`return 0;`
Original file line number	Diff line number	Diff line change
`@@ -333,6 +333,7 @@ int create_bundle(struct bundle_header header, const char path,`
`333`	`333`	`write_or_die(rls.in, sha1_to_hex(object->sha1), 40);`
`334`	`334`	`write_or_die(rls.in, "\n", 1);`
`335`	`335`	`}`
	`336`	`+ close(rls.in);`
`336`	`337`	`if (finish_command(&rls))`
`337`	`338`	`return error ("pack-objects died");`
`338`	`339`
Original file line number	Diff line number	Diff line change
`@@ -132,6 +132,7 @@ static int run_hook(const char *hook_name)`
`132`	`132`	`break;`
`133`	`133`	`}`
`134`	`134`	`}`
	`135`	`+ close(proc.in);`
`135`	`136`	`return hook_status(finish_command(&proc), hook_name);`
`136`	`137`	`}`
`137`	`138`
`@@ -414,6 +415,7 @@ static const char *unpack(void)`
`414`	`415`	`if (start_command(&ip))`
`415`	`416`	`return "index-pack fork failed";`
`416`	`417`	`pack_lockfile = index_pack_lockfile(ip.out);`
	`418`	`+ close(ip.out);`
`417`	`419`	`status = finish_command(&ip);`
`418`	`420`	`if (!status) {`
`419`	`421`	`reprepare_packed_git();`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,6 @@ int start_command(struct child_process *cmd)`
`25`	`25`	`if (pipe(fdin) < 0)`
`26`	`26`	`return -ERR_RUN_COMMAND_PIPE;`
`27`	`27`	`cmd->in = fdin[1];`
`28`		`- cmd->close_in = 1;`
`29`	`28`	`}`
`30`	`29`
`31`	`30`	`need_out = !cmd->no_stdout`
`@@ -38,7 +37,6 @@ int start_command(struct child_process *cmd)`
`38`	`37`	`return -ERR_RUN_COMMAND_PIPE;`
`39`	`38`	`}`
`40`	`39`	`cmd->out = fdout[0];`
`41`		`- cmd->close_out = 1;`
`42`	`40`	`}`
`43`	`41`
`44`	`42`	`need_err = !cmd->no_stderr && cmd->err < 0;`
`@@ -157,10 +155,6 @@ static int wait_or_whine(pid_t pid)`
`157`	`155`
`158`	`156`	`int finish_command(struct child_process *cmd)`
`159`	`157`	`{`
`160`		`- if (cmd->close_in)`
`161`		`- close(cmd->in);`
`162`		`- if (cmd->close_out)`
`163`		`- close(cmd->out);`
`164`	`158`	`return wait_or_whine(cmd->pid);`
`165`	`159`	`}`
`166`	`160`