git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Nov 17, 2014, 23:35 UTC
Message-ID
<20141117233525.GC4336@google.com>
In-Reply-To
<1416262453-30349-1-git-send-email-sbeller@google.com>
Hi,
Stefan Beller wrote:
> This patch was sent previously to the list as part of
> that series[2], but it seems to be unrelated to me.
Thanks.  Good call.
[...]
> From: Ronnie Sahlberg <sahlberg@google.com>
>
> Update copy_fd to return a meaningful errno on failure and also
> preserve the existing errno variable.

Some functions in git make errno meaningful on error and others don't. In general, the more we only use errno immediately after a system call, the better, so based on the above description this seems like a step in the wrong direction.

Do any callers care about errno? Does the function's API documentation say it will make errno meaningful on error, so people making changes to copy_fd in the future know to maintain that property?

*searches*
Looks like callers are:
 convert.c::filter_buffer_or_fd, which doesn't care
 copy.c::copy_file, which also doesn't care
 lockfile.c::hold_lock_file_for_append, which started caring
 in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno
 before returning, 2014-10-01).  But no callers of that function
 care yet.

So this is about fixing a bug-waiting-to-happen in hold_lock_file_for_append. That would be enough to motivate the change.

[...]
>  copy.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Please also update the API documentation in cache.h so we remember not to backslide in the future.

[...]
Show 12 quoted lines
> --- a/copy.c
> +++ b/copy.c
> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
>  		if (!len)
>  			break;
>  		if (len < 0) {
> -			return error("copy-fd: read returned %s",
> -				     strerror(errno));
> +			int save_errno = errno;
> +			error("copy-fd: read returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;

Any caller is presumably going to turn around and print strerror(errno) again, producing repetitive output.

Can we do better?  E.g., if the signature were
	int copy_fd(int ifd, int ofd, struct strbuf *err);

then we could write the error message to the err strbuf for the caller to print. The error handling would be more explicit so there would be no need to protect errno from clobbering by other system calls (both here and in callers).

Something like this:
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
diff --git i/cache.h w/cache.h
index 99ed096..ddaa30f 100644
--- i/cache.h
+++ w/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
 extern void maybe_flush_or_die(FILE *, const char *);
 __attribute__((format (printf, 2, 3)))
 extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern void write_or_die(int fd, const void *buf, size_t count);
diff --git i/convert.c w/convert.c
index 9a5612e..e301447 100644
--- i/convert.c
+++ w/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
 	if (params->src) {
 		write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
 	} else {
-		write_err = copy_fd(params->fd, child_process.in);
+		struct strbuf err = STRBUF_INIT;
+		write_err = copy_fd(params->fd, child_process.in, &err);
+		if (write_err)
+			error("copy-fd: %s", err.buf);
+		strbuf_release(&err);
 	}
 
 	if (close(child_process.in))
diff --git i/copy.c w/copy.c
index f2970ec..828661a 100644
--- i/copy.c
+++ w/copy.c
@@ -1,19 +1,22 @@
 #include "cache.h"
 
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
 {
+	assert(err);
+
 	while (1) {
 		char buffer[8192];
 		ssize_t len = xread(ifd, buffer, sizeof(buffer));
 		if (!len)
 			break;
 		if (len < 0) {
-			return error("copy-fd: read returned %s",
-				     strerror(errno));
+			strbuf_addf(err, "read returned %s", strerror(errno));
+			return -1;
+		}
+		if (write_in_full(ofd, buffer, len) < 0) {
+			strbuf_addf(err, "write returned %s", strerror(errno));
+			return -1;
 		}
-		if (write_in_full(ofd, buffer, len) < 0)
-			return error("copy-fd: write returned %s",
-				     strerror(errno));
 	}
 	return 0;
 }
@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
 
 int copy_file(const char *dst, const char *src, int mode)
 {
-	int fdi, fdo, status;
+	int fdi, fdo;
+	struct strbuf err = STRBUF_INIT;
 
 	mode = (mode & 0111) ? 0777 : 0666;
 	if ((fdi = open(src, O_RDONLY)) < 0)
@@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)
 		close(fdi);
 		return fdo;
 	}
-	status = copy_fd(fdi, fdo);
+	if (copy_fd(fdi, fdo, &err)) {
+		close(fdi);
+		close(fdo);
+		error("copy-fd: %s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
+	strbuf_release(&err);
 	close(fdi);
 	if (close(fdo) != 0)
 		return error("%s: close error: %s", dst, strerror(errno));
-
-	if (!status && adjust_shared_perm(dst))
+	if (adjust_shared_perm(dst))
 		return -1;
 
-	return status;
+	return 0;
 }
 
 int copy_file_with_time(const char *dst, const char *src, int mode)
diff --git i/lockfile.c w/lockfile.c
index 4f16ee7..c47976e 100644
--- i/lockfile.c
+++ w/lockfile.c
@@ -179,37 +179,52 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 	return fd;
 }
 
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
+/*
+ * Like strbuf_addf but inserts at the front of a strbuf instead
+ * of appending.
+ */
+static void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
+{
+	size_t pos, len;
+	va_list ap;
+
+	pos = sb->len;
+
+	va_start(ap, fmt);
+	strbuf_vaddf(sb, fmt, ap);
+	va_end(ap);
+
+	len = sb->len - pos;
+	strbuf_insert(sb, 0, sb->buf + pos, len);
+	strbuf_remove(sb, pos + len, len);
+}
+
+int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+			      int flags, struct strbuf *err)
 {
 	int fd, orig_fd;
 
+	assert(!(flags & LOCK_DIE_ON_ERROR));
+	assert(err && !err->len);
+
 	fd = lock_file(lk, path, flags);
 	if (fd < 0) {
-		if (flags & LOCK_DIE_ON_ERROR)
-			unable_to_lock_die(path, errno);
+		unable_to_lock_message(path, errno, err);
 		return fd;
 	}
 
 	orig_fd = open(path, O_RDONLY);
 	if (orig_fd < 0) {
 		if (errno != ENOENT) {
-			int save_errno = errno;
-
-			if (flags & LOCK_DIE_ON_ERROR)
-				die("cannot open '%s' for copying", path);
+			strbuf_addf(err, "cannot open '%s' for copying: %s",
+				    path, strerror(errno));
 			rollback_lock_file(lk);
-			error("cannot open '%s' for copying", path);
-			errno = save_errno;
 			return -1;
 		}
-	} else if (copy_fd(orig_fd, fd)) {
-		int save_errno = errno;
-
-		if (flags & LOCK_DIE_ON_ERROR)
-			exit(128);
+	} else if (copy_fd(orig_fd, fd, err)) {
+		strbuf_prefixf(err, "cannot copy '%s': ", path);
 		close(orig_fd);
 		rollback_lock_file(lk);
-		errno = save_errno;
 		return -1;
 	} else {
 		close(orig_fd);
diff --git i/lockfile.h w/lockfile.h
index cd2ec95..ca36a1d 100644
--- i/lockfile.h
+++ w/lockfile.h
@@ -75,7 +75,8 @@ extern void unable_to_lock_message(const char *path, int err,
 				   struct strbuf *buf);
 extern NORETURN void unable_to_lock_die(const char *path, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
-extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
+extern int hold_lock_file_for_append(struct lock_file *, const char *path,
+				     int, struct strbuf *err);
 extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);
 extern char *get_locked_file_path(struct lock_file *);
 extern int commit_lock_file_to(struct lock_file *, const char *path);
diff --git i/sha1_file.c w/sha1_file.c
index d7f1838..9ae1839 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -403,14 +403,24 @@ void read_info_alternates(const char * relative_base, int depth)
 
 void add_to_alternates_file(const char *reference)
 {
-	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
-	int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
-	char *alt = mkpath("%s\n", reference);
+	struct lock_file *lock;
+	int fd;
+	char *alt;
+	struct strbuf err = STRBUF_INIT;
+
+	lock = xcalloc(1, sizeof(*lock));
+	fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"),
+				       0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
+	alt = mkpath("%s\n", reference);
 	write_or_die(fd, alt, strlen(alt));
 	if (commit_lock_file(lock))
 		die("could not close alternates file");
 	if (alt_odb_tail)
 		link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
+
+	strbuf_release(&err);
 }
 
 int foreach_alt_odb(alt_odb_fn fn, void *cb)
Previous: Stefan BellerNext: Stefan Beller
Message 4 of 83 in “copy.c: make copy_fd preserve meaningful errno”
  1. copy.c: make copy_fd preserve meaningful errnoStefan Beller, Nov 17, 2014
  2. Junio C HamanoNov 17, 2014
  3. Stefan BellerNov 17, 2014
  4. Jonathan NiederNov 17, 2014
  5. Stefan BellerNov 18, 2014
  6. Jonathan NiederNov 18, 2014
  7. Stefan BellerNov 18, 2014
  8. 0/14 Re: copy.c: make copy_fd preserve meaningful errnoJonathan Nieder, Dec 3, 2014
  9. 01/14 strbuf: introduce strbuf_prefixf()Jonathan Nieder, Dec 3, 2014
  10. Eric SunshineDec 3, 2014
  11. Jonathan NiederDec 3, 2014
  12. Junio C HamanoDec 3, 2014
  13. Jonathan NiederDec 3, 2014
  14. Jonathan NiederDec 3, 2014
  15. Junio C HamanoDec 3, 2014
  16. 02/14 add_to_alternates_file: respect GIT_OBJECT_DIRECTORYJonathan Nieder, Dec 3, 2014
  17. Junio C HamanoDec 3, 2014
  18. 03/14 copy_fd: pass error message back through a strbufJonathan Nieder, Dec 3, 2014
  19. Junio C HamanoDec 3, 2014
  20. Jonathan NiederDec 3, 2014
  21. Jeff KingDec 3, 2014
  22. Jonathan NiederDec 3, 2014
  23. Jeff KingDec 4, 2014
  24. Stefan BellerDec 4, 2014
  25. Jeff KingDec 4, 2014
  26. Michael HaggertyDec 10, 2014
  27. Junio C HamanoDec 10, 2014
  28. Jeff KingDec 10, 2014
  29. doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)Jonathan Nieder, Dec 4, 2014
  30. Junio C HamanoDec 4, 2014
  31. Jonathan NiederDec 4, 2014
  32. Jonathan NiederDec 4, 2014
  33. Jeff KingDec 4, 2014
  34. Junio C HamanoDec 4, 2014
  35. Jeff KingDec 5, 2014
  36. Junio C HamanoDec 5, 2014
  37. Jeff KingDec 7, 2014
  38. Junio C HamanoDec 9, 2014
  39. Jeff KingDec 9, 2014
  40. Junio C HamanoFeb 12, 2015
  41. Michael HaggertyFeb 17, 2015
  42. Junio C HamanoFeb 17, 2015
  43. Jeff KingFeb 17, 2015
  44. Junio C HamanoFeb 17, 2015
  45. Junio C HamanoDec 3, 2014
  46. Jonathan NiederDec 3, 2014
  47. Junio C HamanoDec 3, 2014
  48. Jonathan NiederDec 3, 2014
  49. Stefan BellerDec 3, 2014
  50. 04/14 hold_lock_file_for_append: pass error message back through a strbufJonathan Nieder, Dec 3, 2014
  51. Torsten BögershausenDec 3, 2014
  52. Jonathan NiederDec 3, 2014
  53. 05/14 lock_packed_refs: pass error message back through a strbufJonathan Nieder, Dec 3, 2014
  54. 06/14 lockfile: introduce flag for locks outside .gitJonathan Nieder, Dec 3, 2014
  55. Junio C HamanoDec 3, 2014
  56. Jonathan NiederDec 3, 2014
  57. Junio C HamanoDec 3, 2014
  58. Jonathan NiederDec 3, 2014
  59. Junio C HamanoDec 3, 2014
  60. Jonathan NiederDec 3, 2014
  61. Junio C HamanoDec 3, 2014
  62. Jeff KingDec 3, 2014
  63. Junio C HamanoDec 4, 2014
  64. Jonathan NiederDec 4, 2014
  65. 07/14 fast-import: use message from lockfile API when writing marks failsJonathan Nieder, Dec 3, 2014
  66. 08/14 credentials: use message from lockfile API when locking ~/.git-credentials failsJonathan Nieder, Dec 3, 2014
  67. 09/14 config: use message from lockfile API when locking config file failsJonathan Nieder, Dec 3, 2014
  68. Junio C HamanoDec 3, 2014
  69. Jonathan NiederDec 3, 2014
  70. 10/14 rerere: error out on autoupdate failureJonathan Nieder, Dec 3, 2014
  71. 11/14 hold_locked_index: pass error message back through a strbufJonathan Nieder, Dec 3, 2014
  72. 12/14 hold_lock_file_for_update: pass error message back through a strbufJonathan Nieder, Dec 3, 2014
  73. Jonathan NiederDec 3, 2014
  74. 13/14 lockfile: remove unused function 'unable_to_lock_die'Jonathan Nieder, Dec 3, 2014
  75. 14/14 lockfile: make 'unable_to_lock_message' privateJonathan Nieder, Dec 3, 2014
  76. Stefan BellerDec 3, 2014
  77. Junio C HamanoNov 18, 2014
  78. Junio C HamanoNov 18, 2014
  79. Michael HaggertyNov 21, 2014
  80. Michael HaggertyNov 21, 2014
  81. Junio C HamanoNov 21, 2014
  82. Jeff KingNov 21, 2014
  83. Junio C HamanoNov 21, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.