Re: [PATCH] fast-import.c: detect fclose- and fflush-induced write failure
From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:43:18
Jim Meyering [off-list ref] wrote:
There are potentially ignored write errors in fast-import.c.
...
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.