Re: [PATCH] fast-import.c: detect fclose- and fflush-induced write failure
- From
Shawn O. Pearce <spearce@spearce.org>
- Date
- Jun 25, 2007, 14:12 UTC
- Message-ID
- <20070625141206.GC32223@spearce.org>
- In-Reply-To
- <87abuoxtio.fsf@rho.meyering.net>
Jim Meyering <jim@meyering.net> wrote:
> There are potentially ignored write errors in fast-import.c.
...
Show 9 quoted lines
> 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.
Show 20 quoted lines
> +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.
Show 6 quoted lines
> @@ -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)) {" ?Show 8 quoted lines
> + 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.
Show 5 quoted lines
> @@ -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.
Show 8 quoted lines
> @@ -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.