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

[Jim Meyering <jim@meyering.net>]

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