Jim Meyering <jim@xxxxxxxxxxxx> 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
