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

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

[Jim Meyering]

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 <jim@meyering.net>
---
 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;

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

[Shawn O. Pearce]

Jim Meyering <jim@meyering.net> wrote:
> There are potentially ignored write errors in fast-import.c.
...
> diff --git 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));

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.

> @@ -1369,7 +1396,18 @@ static void dump_marks(void)
>  	}
> 
>  	dump_marks_helper(f, 0, marks);
> -	fclose(f);
> +	if (close_stream(f) != 0) {

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.

> @@ -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;

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.

> @@ -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);

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.

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

[Jim Meyering]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Jim Meyering <jim@meyering.net> wrote:
>> There are potentially ignored write errors in fast-import.c.
> ...
>> diff --git 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));
>
> Hmm.  That's a valid bug, if the disk is full we might not
> be able to flush.

Or if the disk is corrupted.

> But that linewrapping looks pretty ugly.

I agree.
Blame the 8-columns per indentation-level vs 80-col-max guideline.
Besides, the existing indentation level was already pretty deep there.
From the looks of the line just a few above that one, I wonder if the
80-col-max guideline applies to git.

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

Rats.  You're right.
Thanks.  But as I mentioned, close_wstream_or_die is probably not
code that should remain (at least not in its current form) in any case.

>> @@ -1369,7 +1396,18 @@ static void dump_marks(void)
>>  	}
>>
>>  	dump_marks_helper(f, 0, marks);
>> -	fclose(f);
>> +	if (close_stream(f) != 0) {
>
> What about just "if (close_stream(f)) {" ?

Sure.
I use both styles, best to be consistent.

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

Of course.  That's why I proposed to use an error-reporting
function that interpolates an errno value.

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

No.  You seem to be misreading close_stream.
Between the calls to ferror and fclose, errno is not useful.
It's when fclose succeeds yet ferror indicates there was
a preceding error that it sets errno = 0.
And it does that solely because there is already no
guarantee that errno is valid (read ferror's man page),
and this at least makes it so a close_stream caller
does not accidentally use an invalid errno value.

> if we have an error and we're going to explain there was an error
> we should explain what the error was.
>
>> @@ -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;
>
> 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.

Um.  No.
Oh, I see you "got it" below.

>> @@ -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);
>
> 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.

Nor am I.

My first cut at this patch changed it so that there was only one
fclose, and everything was handled *outside* the arg-parsing loop.
But then I noticed that one could use a combination of --import-marks=...
and --export-pack-edges= (with more than one of each) to do what
I presume the original author must have thought was something useful.

IMHO, if it's appropriate to rewrite this code, that should be done
separately from detecting write failure.

Thanks for the feedback.

I'll be happy to redo the patch, once there is consensus on what
it should look like.