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

From: Jim Meyering <jim@meyering.net>
Date: Jun 25, 2007, 19:54 UTC
Message-ID: <871wfzvmgh.fsf@rho.meyering.net>
In-Reply-To: <alpine.LFD.0.98.0706241010480.3593@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 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.

...

> 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.

...

Show 8 quoted lines

> +	/* 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);

Here is a patch relative to "next", to restore some of the functionality that was provided by my initially-proposed change.

>From 379aee16596d29b83c95068964c349399b9b9f47 Mon Sep 17 00:00:00 2001

From: Jim Meyering <jim@meyering.net>
Date: Mon, 25 Jun 2007 18:54:12 +0200
Subject: [PATCH] When detecting write failure, print strerror when possible.

Do not call "die" solely on the basis of ferror. Instead, call both ferror and fclose, and *then* decide whether/how to die.

Without this patch, some commands unnecessarily omit strerror(errno) when they fail:

    $ ./git-ls-tree HEAD > /dev/full
    fatal: write failure on standard output

With the patch, git reports the desired ENOSPC diagnostic:

    fatal: write failure on standard output: No space left on device

FWIW, this version of close_stream is similar to the one I included in another recent patch, but, is slightly cleaner. Also, rather than returning zero or EOF, this one simply returns zero or nonzero.

* git.c (close_stream): New function.
(run_command): Don't die solely because of ferror.  Use close_stream.

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 git.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index b949cbb..b00bb1c 100644
--- a/git.c
+++ b/git.c
@@ -246,6 +246,21 @@ struct cmd_struct {
 	int option;
 };

+static int
+close_stream(FILE *stream)
+{
+	int prev_fail = ferror(stream);
+	int fclose_fail = fclose(stream);
+
+	/* If there was a previous failure, but fclose succeeded,
+	   clear errno, since ferror does not set it, and its value
+	   may be unrelated to the ferror-reported failure.  */
+	if (prev_fail && !fclose_fail)
+		errno = 0;
+
+	return prev_fail || fclose_fail;
+}
+
 static int run_command(struct cmd_struct *p, int argc, const char **argv)
 {
 	int status;
@@ -274,10 +289,13 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 		return 0;

 	/* Check for ENOSPC and EIO errors.. */
-	if (ferror(stdout))
-		die("write failure on standard output");
-	if (fflush(stdout) || fclose(stdout))
-		die("write failure on standard output: %s", strerror(errno));
+	if (close_stream(stdout)) {
+		if (errno == 0)
+			die("write failure on standard output");
+		else
+			die("write failure on standard output: %s",
+			    strerror(errno));
+	}

 	return 0;
 }