threads / patch / 16600

patch, 3 partsMake some of fwrite/fclose/write/close failures visible

Subject: [PATCH 1/3] Make some of fwrite/fclose/write/close failures visible

## tl;dr

4 messages between Dec 5, 2008 and Dec 5, 2008. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Alex Riesen· Dec 5, 2008, 00:35 UTC · lore

So that full filesystem conditions or permissions problems wont go unnoticed.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

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(-)
Show changes to 3 files +45 −15

builtin-fsck.c, builtin-merge.c, rerere.c

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· Dec 5, 2008, 00:36 UTC · re: Alex Riesen · lore

[PATCH 2/3] Make chdir failures visible

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 git.c   |    4 ++--
 setup.c |    3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 2 files +4 −3

git.c, setup.c

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· Dec 5, 2008, 00:39 UTC · re: Alex Riesen · lore

[PATCH 3/3] Report symlink failures in merge-recursive

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

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(-)
Show changes to merge-recursive.c +2 −1
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

← back to recent threads