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

["Shawn O. Pearce" <spearce@spearce.org>]

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.