[PATCH] fast-import.c: detect fclose- and fflush-induced write failure

There are potentially ignored write errors in fast-import.c.
Here's a proof-of-concept patch.
Something like close_stream might be worth making more generally
accessible, but rather than close_wstream_or_die, I'd prefer general
purpose die-like functions that take an errno value, e.g.,

  die_errno (errno, fmt_str, arg1, ...)

that would work just like die, with this change:
- when errno is nonzero,
    append the concatenation of ": " and strerror(errno)
    between the format-string-denoted output and the final newline
- when errno is zero
    work just like "die" currently does.

This functionality is like that provided by the error, warn*,
err, verr, etc. functions that have been in glibc forever.

With such a function (and an error_errno analog), the nearly-identical
uses of "error" added below and the nearly identical uses of "die" that
might end up in git.c could be factored out.

Here's a ChangeLog-style entry:

 (end_packfile): Die upon fflush failure.
 (close_stream, close_wstream_or_die): New functions.
 (dump_marks): Upon fclose failure, rollback the lock and give a diagnostic.
 (main): Die upon fclose failure.  Record pack_edges file name for use in diagnostics.

Signed-off-by: Jim Meyering <redacted>
---
 fast-import.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index f9bfcc7..7941839 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -793,7 +793,9 @@ static void end_packfile(void)
 					fprintf(pack_edges, " %s", sha1_to_hex(t->sha1));
 			}
 			fputc('\n', pack_edges);
-			fflush(pack_edges);
+			if (fflush(pack_edges))
+				die("failed to write pack-edges file: %s",
+				    strerror(errno));
 		}

 		pack_id++;

@@ -1344,6 +1346,31 @@ static void dump_marks_helper(FILE *f,
 	}
 }

+static int
+close_stream(FILE *stream)
+{
+	int prev_fail = (ferror(stream) != 0);
+	int fclose_fail = (fclose(stream) != 0);
+
+	if (prev_fail || fclose_fail) {
+		if (! fclose_fail)
+			errno = 0;
+		return EOF;
+	}
+	return 0;
+}
+
+static void
+close_wstream_or_die(FILE *stream, const char *file_name)
+{
+	if (close_stream(stream)) {
+		if (errno == 0)
+			die ("%s: write failed: %s", file_name, strerror(errno));
+		else
+			die ("%s: write failed", file_name);
+	}
+}
+
 static void dump_marks(void)
 {
 	static struct lock_file mark_lock;

@@ -1369,7 +1396,18 @@ static void dump_marks(void)
 	}

 	dump_marks_helper(f, 0, marks);
-	fclose(f);
+	if (close_stream(f) != 0) {
+		int close_errno = errno;
+		rollback_lock_file(&mark_lock);
+		failure |=
+		  (close_errno == 0
+		   ? error("Failed to write temporary marks file %s.lock",
+			   mark_file)
+		   : error("Failed to write temporary marks file %s.lock: %s",
+			   mark_file, strerror(close_errno)));
+		return;
+	}
+
 	if (commit_lock_file(&mark_lock))
 		failure |= error("Unable to write marks file %s: %s",
 			mark_file, strerror(errno));

@@ -2015,6 +2053,7 @@ static const char fast_import_usage[] =
 int main(int argc, const char **argv)
 {
 	int i, show_stats = 1;
+	const char *pack_edges_file = NULL;

 	git_config(git_default_config);
 	alloc_objects(object_entry_alloc);

@@ -2052,10 +2091,13 @@ int main(int argc, const char **argv)
 			mark_file = a + 15;
 		else if (!prefixcmp(a, "--export-pack-edges=")) {
 			if (pack_edges)
-				fclose(pack_edges);
-			pack_edges = fopen(a + 20, "a");
+				close_wstream_or_die(pack_edges,
+						     pack_edges_file);
+			pack_edges_file = a + 20;
+			pack_edges = fopen(pack_edges_file, "a");
 			if (!pack_edges)
-				die("Cannot open %s: %s", a + 20, strerror(errno));
+				die("Cannot open %s: %s", pack_edges_file,
+				    strerror(errno));
 		} else if (!strcmp(a, "--force"))
 			force_update = 1;
 		else if (!strcmp(a, "--quiet"))

@@ -2095,7 +2137,7 @@ int main(int argc, const char **argv)
 	dump_marks();

 	if (pack_edges)
-		fclose(pack_edges);
+		close_wstream_or_die(pack_edges, pack_edges_file);

 	if (show_stats) {
 		uintmax_t total_count = 0, duplicate_count = 0;

Jim Meyering [off-list ref] wrote:

There are potentially ignored write errors in fast-import.c.

...

Hmm.  That's a valid bug, if the disk is full we might not
be able to flush.  But that linewrapping looks pretty ugly.
 

+static int
+close_stream(FILE *stream)
+{
+	int prev_fail = (ferror(stream) != 0);
+	int fclose_fail = (fclose(stream) != 0);
+
+	if (prev_fail || fclose_fail) {
+		if (! fclose_fail)
+			errno = 0;
+		return EOF;
+	}
+	return 0;
+}
+
+static void
+close_wstream_or_die(FILE *stream, const char *file_name)
+{
+	if (close_stream(stream)) {
+		if (errno == 0)
+			die ("%s: write failed: %s", file_name, strerror(errno));

Don't you mean "if (errno != 0)" here?  Right now you are printing
"No Error" when there is no error and tossing the errno when there
is an error.

What about just "if (close_stream(f)) {" ?

+		int close_errno = errno;
+		rollback_lock_file(&mark_lock);
+		failure |=
+		  (close_errno == 0
+		   ? error("Failed to write temporary marks file %s.lock",
+			   mark_file)
+		   : error("Failed to write temporary marks file %s.lock: %s",
+			   mark_file, strerror(close_errno)));

Ugh.  The ternary operator has many uses, but using it to decide
which error() function you are going to call and have both cases
bit-wise or a -1 into failure is not one of them.  This would be
a lot cleaner if the ternary operator wasn't abused here.

And looking at this code I'm now wondering about the code above
for close_stream().  If it returns EOF but doesn't supply a valid
errno its because you tossed the errno that was available when you
did the fclose().  So I'd actually say the close_stream() is bad;
if we have an error and we're going to explain there was an error
we should explain what the error was.

This is only ever used in the "--export-pack-edges=" arm of the
option parser.  It should be local to that block, not to the
entire function.

Oh, I see, its actually being reused to issue an error that we
couldn't close the prior file.  The more that I look at this we
probably should just die() if we get a second arg with this option.
What does it mean to give --export-pack-edges twice?  Apparently
under the current code it means use the last one, but I'm not sure
that's sane.

-- 
Shawn.