# [PATCH 1/3] Make some of fwrite/fclose/write/close failures visible 4 messages from 2008-12-05 to 2008-12-05. Participants: Alex Riesen, Junio C Hamano. Thread: https://gitlist.dev/t/16600 ## Alex Riesen, 2008-12-05 00:35 Subject: [PATCH 1/3] Make some of fwrite/fclose/write/close failures visible Message-ID: <20081205003546.GA7294@blimp.localdomain> URL: https://gitlist.dev/e/20081205003546.GA7294%40blimp.localdomain ``` So that full filesystem conditions or permissions problems wont go unnoticed. Signed-off-by: Alex Riesen --- This and the follow-up patches is fallout of Windows debugging sessions. I implemented random error handling code just to figure out where it might be going wrong. None of that code actually helped to fix something (and lots was just thrown away), but some, I think, still makes sense. This patch adds error handling only to fwrite/fputs where the return value was ignored and writing was definitely into a file. BTW, libc headers in Ubuntu 8.10 have warn_unused_result attribute added to many functions (fwrite(3) and write(2) amongst them). This helps finding the problem places. builtin-fsck.c | 8 ++++++-- builtin-merge.c | 6 ++++-- rerere.c | 46 +++++++++++++++++++++++++++++++++++----------- 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/builtin-fsck.c b/builtin-fsck.c index d3f3de9..afded5e 100644 --- a/builtin-fsck.c +++ b/builtin-fsck.c @@ -201,12 +201,16 @@ static void check_unreachable_object(struct object *obj) char *buf = read_sha1_file(obj->sha1, &type, &size); if (buf) { - fwrite(buf, size, 1, f); + if (fwrite(buf, size, 1, f) != 1) + die("Could not write %s: %s", + filename, strerror(errno)); free(buf); } } else fprintf(f, "%s\n", sha1_to_hex(obj->sha1)); - fclose(f); + if (fclose(f)) + die("Could not finish %s: %s", + filename, strerror(errno)); } return; } diff --git a/builtin-merge.c b/builtin-merge.c index 7c2b90c..cf86975 100644 --- a/builtin-merge.c +++ b/builtin-merge.c @@ -293,8 +293,10 @@ static void squash_message(void) pretty_print_commit(rev.commit_format, commit, &out, rev.abbrev, NULL, NULL, rev.date_mode, 0); } - write(fd, out.buf, out.len); - close(fd); + if (write(fd, out.buf, out.len) < 0) + die("Writing SQUASH_MSG: %s", strerror(errno)); + if (close(fd)) + die("Finishing SQUASH_MSG: %s", strerror(errno)); strbuf_release(&out); } diff --git a/rerere.c b/rerere.c index 02931a1..718fb52 100644 --- a/rerere.c +++ b/rerere.c @@ -70,6 +70,19 @@ static int write_rr(struct string_list *rr, int out_fd) return 0; } +static void ferr_write(const void *p, size_t count, FILE *fp, int *err) +{ + if (!count || *err) + return; + if (fwrite(p, count, 1, fp) != 1) + *err = errno; +} + +static inline void ferr_puts(const char *s, FILE *fp, int *err) +{ + ferr_write(s, strlen(s), fp, err); +} + static int handle_file(const char *path, unsigned char *sha1, const char *output) { @@ -82,6 +95,7 @@ static int handle_file(const char *path, struct strbuf one = STRBUF_INIT, two = STRBUF_INIT; FILE *f = fopen(path, "r"); FILE *out = NULL; + int wrerror = 0; if (!f) return error("Could not open %s", path); @@ -118,11 +132,11 @@ static int handle_file(const char *path, hunk_no++; hunk = RR_CONTEXT; if (out) { - fputs("<<<<<<<\n", out); - fwrite(one.buf, one.len, 1, out); - fputs("=======\n", out); - fwrite(two.buf, two.len, 1, out); - fputs(">>>>>>>\n", out); + ferr_puts("<<<<<<<\n", out, &wrerror); + ferr_write(one.buf, one.len, out, &wrerror); + ferr_puts("=======\n", out, &wrerror); + ferr_write(two.buf, two.len, out, &wrerror); + ferr_puts(">>>>>>>\n", out, &wrerror); } if (sha1) { git_SHA1_Update(&ctx, one.buf ? one.buf : "", @@ -139,7 +153,7 @@ static int handle_file(const char *path, else if (hunk == RR_SIDE_2) strbuf_addstr(&two, buf); else if (out) - fputs(buf, out); + ferr_puts(buf, out, &wrerror); continue; bad: hunk = 99; /* force error exit */ @@ -149,8 +163,12 @@ static int handle_file(const char *path, strbuf_release(&two); fclose(f); - if (out) - fclose(out); + if (wrerror) + error("There were errors while writing %s (%s)", + path, strerror(wrerror)); + if (out && fclose(out)) + wrerror = error("Failed to flush %s: %s", + path, strerror(errno)); if (sha1) git_SHA1_Final(sha1, &ctx); if (hunk != RR_CONTEXT) { @@ -158,6 +176,8 @@ static int handle_file(const char *path, unlink(output); return error("Could not parse conflict hunks in %s", path); } + if (wrerror) + return -1; return hunk_no; } @@ -200,9 +220,13 @@ static int merge(const char *name, const char *path) if (!ret) { FILE *f = fopen(path, "w"); if (!f) - return error("Could not write to %s", path); - fwrite(result.ptr, result.size, 1, f); - fclose(f); + return error("Could not open %s: %s", path, + strerror(errno)); + if (fwrite(result.ptr, result.size, 1, f) != 1) + error("Could not write %s: %s", path, strerror(errno)); + if (fclose(f)) + return error("Writing %s failed: %s", path, + strerror(errno)); } free(cur.ptr); -- 1.6.1.rc1.29.gb140 ``` ## Alex Riesen, 2008-12-05 00:36 Subject: [PATCH 2/3] Make chdir failures visible Message-ID: <20081205003646.GB7294@blimp.localdomain> URL: https://gitlist.dev/e/20081205003646.GB7294%40blimp.localdomain In-Reply-To: <20081205003546.GA7294@blimp.localdomain> ``` Signed-off-by: Alex Riesen --- git.c | 4 ++-- setup.c | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/git.c b/git.c index 9e5813c..940a498 100644 --- a/git.c +++ b/git.c @@ -195,8 +195,8 @@ static int handle_alias(int *argcp, const char ***argv) ret = 1; } - if (subdir) - chdir(subdir); + if (subdir && chdir(subdir)) + die("Cannot change to %s: %s", subdir, strerror(errno)); errno = saved_errno; diff --git a/setup.c b/setup.c index 78a8041..833ced2 100644 --- a/setup.c +++ b/setup.c @@ -470,7 +470,8 @@ const char *setup_git_directory_gently(int *nongit_ok) } die("Not a git repository"); } - chdir(".."); + if (chdir("..")) + die("Cannot change to %s/..: %s", cwd, strerror(errno)); } inside_git_dir = 0; -- 1.6.1.rc1.29.gb140 ``` ## Alex Riesen, 2008-12-05 00:39 Subject: [PATCH 3/3] Report symlink failures in merge-recursive Message-ID: <20081205003914.GC7294@blimp.localdomain> URL: https://gitlist.dev/e/20081205003914.GC7294%40blimp.localdomain In-Reply-To: <20081205003546.GA7294@blimp.localdomain> ``` Signed-off-by: Alex Riesen --- I don't check for unlink success in the line above, because symlink will fail if unlink failed to cleanup the new path. merge-recursive.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 0e988f2..a0c804c 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -525,7 +525,8 @@ static void update_file_flags(struct merge_options *o, char *lnk = xmemdupz(buf, size); safe_create_leading_directories_const(path); unlink(path); - symlink(lnk, path); + if (symlink(lnk, path)) + die("failed to symlink %s: %s", path, strerror(errno)); free(lnk); } else die("do not know what to do with %06o %s '%s'", -- 1.6.1.rc1.29.gb140 ``` ## Junio C Hamano, 2008-12-05 02:04 Subject: Re: [PATCH 1/3] Make some of fwrite/fclose/write/close failures visible Message-ID: <7v63lz2x8c.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v63lz2x8c.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20081205003546.GA7294@blimp.localdomain> ``` All of them obviously look correct. Will apply to the tip of master. Thanks. ```