{"thread":{"id":"8711","subject":"[PATCH] fast-import.c: detect fclose- and fflush-induced write failure","startedAt":"2007-06-25T09:39:27Z","lastAt":"2007-06-25T14:33:47Z","messageCount":3,"participants":["Jim Meyering","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"45755","messageId":"87abuoxtio.fsf@rho.meyering.net","threadId":"8711","inReplyTo":null,"subject":"[PATCH] fast-import.c: detect fclose- and fflush-induced write failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T09:39:27Z","receivedAt":"2007-06-25T09:39:27Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"There are potentially ignored write errors in fast-import.c.\nHere's a proof-of-concept patch.\nSomething like close_stream might be worth making more generally\naccessible, but rather than close_wstream_or_die, I'd prefer general\npurpose die-like functions that take an errno value, e.g.,\n\n  die_errno (errno, fmt_str, arg1, ...)\n\nthat would work just like die, with this change:\n- when errno is nonzero,\n    append the concatenation of \": \" and strerror(errno)\n    between the format-string-denoted output and the final newline\n- when errno is zero\n    work just like \"die\" currently does.\n\nThis functionality is like that provided by the error, warn*,\nerr, verr, etc. functions that have been in glibc forever.\n\nWith such a function (and an error_errno analog), the nearly-identical\nuses of \"error\" added below and the nearly identical uses of \"die\" that\nmight end up in git.c could be factored out.\n\nHere's a ChangeLog-style entry:\n\n (end_packfile): Die upon fflush failure.\n (close_stream, close_wstream_or_die): New functions.\n (dump_marks): Upon fclose failure, rollback the lock and give a diagnostic.\n (main): Die upon fclose failure.  Record pack_edges file name for use in diagnostics.\n\nSigned-off-by: Jim Meyering <jim@meyering.net>\n---\n fast-import.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++------\n 1 files changed, 48 insertions(+), 6 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex f9bfcc7..7941839 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -793,7 +793,9 @@ static void end_packfile(void)\n \t\t\t\t\tfprintf(pack_edges, \" %s\", sha1_to_hex(t->sha1));\n \t\t\t}\n \t\t\tfputc('\\n', pack_edges);\n-\t\t\tfflush(pack_edges);\n+\t\t\tif (fflush(pack_edges))\n+\t\t\t\tdie(\"failed to write pack-edges file: %s\",\n+\t\t\t\t    strerror(errno));\n \t\t}\n\n \t\tpack_id++;\n@@ -1344,6 +1346,31 @@ static void dump_marks_helper(FILE *f,\n \t}\n }\n\n+static int\n+close_stream(FILE *stream)\n+{\n+\tint prev_fail = (ferror(stream) != 0);\n+\tint fclose_fail = (fclose(stream) != 0);\n+\n+\tif (prev_fail || fclose_fail) {\n+\t\tif (! fclose_fail)\n+\t\t\terrno = 0;\n+\t\treturn EOF;\n+\t}\n+\treturn 0;\n+}\n+\n+static void\n+close_wstream_or_die(FILE *stream, const char *file_name)\n+{\n+\tif (close_stream(stream)) {\n+\t\tif (errno == 0)\n+\t\t\tdie (\"%s: write failed: %s\", file_name, strerror(errno));\n+\t\telse\n+\t\t\tdie (\"%s: write failed\", file_name);\n+\t}\n+}\n+\n static void dump_marks(void)\n {\n \tstatic struct lock_file mark_lock;\n@@ -1369,7 +1396,18 @@ static void dump_marks(void)\n \t}\n\n \tdump_marks_helper(f, 0, marks);\n-\tfclose(f);\n+\tif (close_stream(f) != 0) {\n+\t\tint close_errno = errno;\n+\t\trollback_lock_file(&mark_lock);\n+\t\tfailure |=\n+\t\t  (close_errno == 0\n+\t\t   ? error(\"Failed to write temporary marks file %s.lock\",\n+\t\t\t   mark_file)\n+\t\t   : error(\"Failed to write temporary marks file %s.lock: %s\",\n+\t\t\t   mark_file, strerror(close_errno)));\n+\t\treturn;\n+\t}\n+\n \tif (commit_lock_file(&mark_lock))\n \t\tfailure |= error(\"Unable to write marks file %s: %s\",\n \t\t\tmark_file, strerror(errno));\n@@ -2015,6 +2053,7 @@ static const char fast_import_usage[] =\n int main(int argc, const char **argv)\n {\n \tint i, show_stats = 1;\n+\tconst char *pack_edges_file = NULL;\n\n \tgit_config(git_default_config);\n \talloc_objects(object_entry_alloc);\n@@ -2052,10 +2091,13 @@ int main(int argc, const char **argv)\n \t\t\tmark_file = a + 15;\n \t\telse if (!prefixcmp(a, \"--export-pack-edges=\")) {\n \t\t\tif (pack_edges)\n-\t\t\t\tfclose(pack_edges);\n-\t\t\tpack_edges = fopen(a + 20, \"a\");\n+\t\t\t\tclose_wstream_or_die(pack_edges,\n+\t\t\t\t\t\t     pack_edges_file);\n+\t\t\tpack_edges_file = a + 20;\n+\t\t\tpack_edges = fopen(pack_edges_file, \"a\");\n \t\t\tif (!pack_edges)\n-\t\t\t\tdie(\"Cannot open %s: %s\", a + 20, strerror(errno));\n+\t\t\t\tdie(\"Cannot open %s: %s\", pack_edges_file,\n+\t\t\t\t    strerror(errno));\n \t\t} else if (!strcmp(a, \"--force\"))\n \t\t\tforce_update = 1;\n \t\telse if (!strcmp(a, \"--quiet\"))\n@@ -2095,7 +2137,7 @@ int main(int argc, const char **argv)\n \tdump_marks();\n\n \tif (pack_edges)\n-\t\tfclose(pack_edges);\n+\t\tclose_wstream_or_die(pack_edges, pack_edges_file);\n\n \tif (show_stats) {\n \t\tuintmax_t total_count = 0, duplicate_count = 0;\n"},{"id":"45771","messageId":"20070625141206.GC32223@spearce.org","threadId":"8711","inReplyTo":"87abuoxtio.fsf@rho.meyering.net","subject":"Re: [PATCH] fast-import.c: detect fclose- and fflush-induced write failure","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-06-25T14:12:06Z","receivedAt":"2007-06-25T14:12:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jim Meyering <jim@meyering.net> wrote:\n> There are potentially ignored write errors in fast-import.c.\n...\n> diff --git a/fast-import.c b/fast-import.c\n> @@ -793,7 +793,9 @@ static void end_packfile(void)\n>  \t\t\t\t\tfprintf(pack_edges, \" %s\", sha1_to_hex(t->sha1));\n>  \t\t\t}\n>  \t\t\tfputc('\\n', pack_edges);\n> -\t\t\tfflush(pack_edges);\n> +\t\t\tif (fflush(pack_edges))\n> +\t\t\t\tdie(\"failed to write pack-edges file: %s\",\n> +\t\t\t\t    strerror(errno));\n\nHmm.  That's a valid bug, if the disk is full we might not\nbe able to flush.  But that linewrapping looks pretty ugly.\n \n> +static int\n> +close_stream(FILE *stream)\n> +{\n> +\tint prev_fail = (ferror(stream) != 0);\n> +\tint fclose_fail = (fclose(stream) != 0);\n> +\n> +\tif (prev_fail || fclose_fail) {\n> +\t\tif (! fclose_fail)\n> +\t\t\terrno = 0;\n> +\t\treturn EOF;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static void\n> +close_wstream_or_die(FILE *stream, const char *file_name)\n> +{\n> +\tif (close_stream(stream)) {\n> +\t\tif (errno == 0)\n> +\t\t\tdie (\"%s: write failed: %s\", file_name, strerror(errno));\n\nDon't you mean \"if (errno != 0)\" here?  Right now you are printing\n\"No Error\" when there is no error and tossing the errno when there\nis an error.\n\n> @@ -1369,7 +1396,18 @@ static void dump_marks(void)\n>  \t}\n> \n>  \tdump_marks_helper(f, 0, marks);\n> -\tfclose(f);\n> +\tif (close_stream(f) != 0) {\n\nWhat about just \"if (close_stream(f)) {\" ?\n\n> +\t\tint close_errno = errno;\n> +\t\trollback_lock_file(&mark_lock);\n> +\t\tfailure |=\n> +\t\t  (close_errno == 0\n> +\t\t   ? error(\"Failed to write temporary marks file %s.lock\",\n> +\t\t\t   mark_file)\n> +\t\t   : error(\"Failed to write temporary marks file %s.lock: %s\",\n> +\t\t\t   mark_file, strerror(close_errno)));\n\nUgh.  The ternary operator has many uses, but using it to decide\nwhich error() function you are going to call and have both cases\nbit-wise or a -1 into failure is not one of them.  This would be\na lot cleaner if the ternary operator wasn't abused here.\n\nAnd looking at this code I'm now wondering about the code above\nfor close_stream().  If it returns EOF but doesn't supply a valid\nerrno its because you tossed the errno that was available when you\ndid the fclose().  So I'd actually say the close_stream() is bad;\nif we have an error and we're going to explain there was an error\nwe should explain what the error was.\n\n> @@ -2015,6 +2053,7 @@ static const char fast_import_usage[] =\n>  int main(int argc, const char **argv)\n>  {\n>  \tint i, show_stats = 1;\n> +\tconst char *pack_edges_file = NULL;\n\nThis is only ever used in the \"--export-pack-edges=\" arm of the\noption parser.  It should be local to that block, not to the\nentire function.\n\n> @@ -2052,10 +2091,13 @@ int main(int argc, const char **argv)\n>  \t\t\tmark_file = a + 15;\n>  \t\telse if (!prefixcmp(a, \"--export-pack-edges=\")) {\n>  \t\t\tif (pack_edges)\n> -\t\t\t\tfclose(pack_edges);\n> -\t\t\tpack_edges = fopen(a + 20, \"a\");\n> +\t\t\t\tclose_wstream_or_die(pack_edges,\n> +\t\t\t\t\t\t     pack_edges_file);\n\nOh, I see, its actually being reused to issue an error that we\ncouldn't close the prior file.  The more that I look at this we\nprobably should just die() if we get a second arg with this option.\nWhat does it mean to give --export-pack-edges twice?  Apparently\nunder the current code it means use the last one, but I'm not sure\nthat's sane.\n\n-- \nShawn.\n"},{"id":"45773","messageId":"87vedcumr8.fsf@rho.meyering.net","threadId":"8711","inReplyTo":"20070625141206.GC32223@spearce.org","subject":"Re: [PATCH] fast-import.c: detect fclose- and fflush-induced write failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T14:33:47Z","receivedAt":"2007-06-25T14:33:47Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> Jim Meyering <jim@meyering.net> wrote:\n>> There are potentially ignored write errors in fast-import.c.\n> ...\n>> diff --git a/fast-import.c b/fast-import.c\n>> @@ -793,7 +793,9 @@ static void end_packfile(void)\n>>  \t\t\t\t\tfprintf(pack_edges, \" %s\", sha1_to_hex(t->sha1));\n>>  \t\t\t}\n>>  \t\t\tfputc('\\n', pack_edges);\n>> -\t\t\tfflush(pack_edges);\n>> +\t\t\tif (fflush(pack_edges))\n>> +\t\t\t\tdie(\"failed to write pack-edges file: %s\",\n>> +\t\t\t\t    strerror(errno));\n>\n> Hmm.  That's a valid bug, if the disk is full we might not\n> be able to flush.\n\nOr if the disk is corrupted.\n\n> But that linewrapping looks pretty ugly.\n\nI agree.\nBlame the 8-columns per indentation-level vs 80-col-max guideline.\nBesides, the existing indentation level was already pretty deep there.\nFrom the looks of the line just a few above that one, I wonder if the\n80-col-max guideline applies to git.\n\n>> +static int\n>> +close_stream(FILE *stream)\n>> +{\n>> +\tint prev_fail = (ferror(stream) != 0);\n>> +\tint fclose_fail = (fclose(stream) != 0);\n>> +\n>> +\tif (prev_fail || fclose_fail) {\n>> +\t\tif (! fclose_fail)\n>> +\t\t\terrno = 0;\n>> +\t\treturn EOF;\n>> +\t}\n>> +\treturn 0;\n>> +}\n>> +\n>> +static void\n>> +close_wstream_or_die(FILE *stream, const char *file_name)\n>> +{\n>> +\tif (close_stream(stream)) {\n>> +\t\tif (errno == 0)\n>> +\t\t\tdie (\"%s: write failed: %s\", file_name, strerror(errno));\n>\n> Don't you mean \"if (errno != 0)\" here?  Right now you are printing\n> \"No Error\" when there is no error and tossing the errno when there\n> is an error.\n\nRats.  You're right.\nThanks.  But as I mentioned, close_wstream_or_die is probably not\ncode that should remain (at least not in its current form) in any case.\n\n>> @@ -1369,7 +1396,18 @@ static void dump_marks(void)\n>>  \t}\n>>\n>>  \tdump_marks_helper(f, 0, marks);\n>> -\tfclose(f);\n>> +\tif (close_stream(f) != 0) {\n>\n> What about just \"if (close_stream(f)) {\" ?\n\nSure.\nI use both styles, best to be consistent.\n\n>> +\t\tint close_errno = errno;\n>> +\t\trollback_lock_file(&mark_lock);\n>> +\t\tfailure |=\n>> +\t\t  (close_errno == 0\n>> +\t\t   ? error(\"Failed to write temporary marks file %s.lock\",\n>> +\t\t\t   mark_file)\n>> +\t\t   : error(\"Failed to write temporary marks file %s.lock: %s\",\n>> +\t\t\t   mark_file, strerror(close_errno)));\n>\n> Ugh.  The ternary operator has many uses, but using it to decide\n> which error() function you are going to call and have both cases\n> bit-wise or a -1 into failure is not one of them.  This would be\n> a lot cleaner if the ternary operator wasn't abused here.\n\nOf course.  That's why I proposed to use an error-reporting\nfunction that interpolates an errno value.\n\n> And looking at this code I'm now wondering about the code above\n> for close_stream().  If it returns EOF but doesn't supply a valid\n> errno its because you tossed the errno that was available when you\n> did the fclose().  So I'd actually say the close_stream() is bad;\n\nNo.  You seem to be misreading close_stream.\nBetween the calls to ferror and fclose, errno is not useful.\nIt's when fclose succeeds yet ferror indicates there was\na preceding error that it sets errno = 0.\nAnd it does that solely because there is already no\nguarantee that errno is valid (read ferror's man page),\nand this at least makes it so a close_stream caller\ndoes not accidentally use an invalid errno value.\n\n> if we have an error and we're going to explain there was an error\n> we should explain what the error was.\n>\n>> @@ -2015,6 +2053,7 @@ static const char fast_import_usage[] =\n>>  int main(int argc, const char **argv)\n>>  {\n>>  \tint i, show_stats = 1;\n>> +\tconst char *pack_edges_file = NULL;\n>\n> This is only ever used in the \"--export-pack-edges=\" arm of the\n> option parser.  It should be local to that block, not to the\n> entire function.\n\nUm.  No.\nOh, I see you \"got it\" below.\n\n>> @@ -2052,10 +2091,13 @@ int main(int argc, const char **argv)\n>>  \t\t\tmark_file = a + 15;\n>>  \t\telse if (!prefixcmp(a, \"--export-pack-edges=\")) {\n>>  \t\t\tif (pack_edges)\n>> -\t\t\t\tfclose(pack_edges);\n>> -\t\t\tpack_edges = fopen(a + 20, \"a\");\n>> +\t\t\t\tclose_wstream_or_die(pack_edges,\n>> +\t\t\t\t\t\t     pack_edges_file);\n>\n> Oh, I see, its actually being reused to issue an error that we\n> couldn't close the prior file.  The more that I look at this we\n> probably should just die() if we get a second arg with this option.\n> What does it mean to give --export-pack-edges twice?  Apparently\n> under the current code it means use the last one, but I'm not sure\n> that's sane.\n\nNor am I.\n\nMy first cut at this patch changed it so that there was only one\nfclose, and everything was handled *outside* the arg-parsing loop.\nBut then I noticed that one could use a combination of --import-marks=...\nand --export-pack-edges= (with more than one of each) to do what\nI presume the original author must have thought was something useful.\n\nIMHO, if it's appropriate to rewrite this code, that should be done\nseparately from detecting write failure.\n\nThanks for the feedback.\n\nI'll be happy to redo the patch, once there is consensus on what\nit should look like.\n"}]}