[PATCH 2/2] Check for IO errors after running a command

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jun 24, 2007, 17:29 UTC
Message-ID: <alpine.LFD.0.98.0706241010480.3593@woody.linux-foundation.org>
In-Reply-To: <alpine.LFD.0.98.0706240951440.3593@woody.linux-foundation.org>

This is trying to implement the strict IO error checks that Jim Meyering suggested, but explicitly limits it to just regular files. If a pipe gets closed on us, we shouldn't complain about it.

[ Side note: feel free to change the S_ISREG() to a !S_ISFIFO(). That 
  would allow easier testing with /dev/full, which tends to be a character 
  special device. That said, I think sockets and pipes are generally 
  interchangeable, which is why I limited it to just regular files. ]

If the subcommand already returned an error, that takes precedence (and we assume that the subcommand already printed out any relevant messages relating to it)

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Hmm? I'm not saying this is the only way to do this, but I think this is at least likely to be obviously just an improvement, and it leaves room for further tweaking of the logic if Jim or others find other cases that should be handled.

Side note: I think I made a mistake in making the run_command() a NORETURN function and putting the exit() into it. It's probably better to instead just make it return "int", and make the caller do

	exit(run_command(...));

and that makes it much prettier to have "run_command()" just return early if an error happens (or doesn't happen).

For example, then we could just do

	status = p->fn(...);
	if (status)
		return status;
	/* Somebody closed stdout? */
	if (fstat(fileno(stdout), &st))
		return 0;
	/* Ignore write errors for pipes and sockets.. */
	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
		return 0;

which makes it easy to explain what's going on, and avoids having any deep indentation at all.

I dunno. This passes all the tests, but it's not like we currently test for ENOSPC/EIO anyway, or even can do that. If we _just_ disable the thing for pipes/sockets, we could add a test using /dev/full.

I did check that changing it to !S_ISFIFO() gets the right behaviour, and "git log | head" doesn't complain, while "git log > /dev/full" does. So this has gotten some very rudimentary testing, but not in the exact form I'm actually sending it out.

 git.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 6c728e4..db0118a 100644
--- a/git.c
+++ b/git.c
@@ -224,6 +224,8 @@ struct cmd_struct {
 
 static NORETURN void run_command(struct cmd_struct *p, int argc, const char **argv)
 {
+	int status;
+	struct stat st;
 	const char *prefix;
 
 	prefix = NULL;
@@ -237,7 +239,18 @@ static NORETURN void run_command(struct cmd_struct *p, int argc, const char **ar
 	}
 	trace_argv_printf(argv, argc, "trace: built-in: git");
 
-	exit(p->fn(argc, argv, prefix));
+	status = p->fn(argc, argv, prefix);
+	if (status)
+		exit(status);
+
+	/* Check for ENOSPC and EIO errors.. */
+	if (!fstat(fileno(stdout), &st) && S_ISREG(st.st_mode)) {
+		if (ferror(stdout))
+			die("write failure on standard output");
+		if (fflush(stdout) || fclose(stdout))
+			die("write failure on standard output: %s", strerror(errno));
+	}
+	exit(0);
 }
 
 static void handle_internal_command(int argc, const char **argv)