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

[PATCH 7/7] add xread_in_full() helper

From
Jeff King <peff@peff.net>
Date
Sep 25, 2017, 20:33 UTC
Message-ID
<20170925203323.rmm3i5kx266ma3wu@sigill.intra.peff.net>
In-Reply-To
<20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net>

Many callers of read_in_full() expect to see exactly "len" bytes, and die if that isn't the case. Since there are two interesting error cases:

  1. We saw a read() error.
  2. We saw EOF with fewer bytes than expected.

we would ideally distinguish between them when reporting to the user. Let's add a helper to make this easier. We can convert cases that currently lump the two conditions together (improving their error messages) and ones that already distinguish (shortening their code).

There are a number of read_in_full() calls left that we can't (or shouldn't) convert, for one or more of these reasons:

 - Some are not trying to read an exact number in the first
   place. So they only care about case 1, and are fine.
 - Some call error() instead of die(). We could possibly
   accommodate these, at the expense of making our helper a
   bit more clunky.
 - Some print nothing and return a failing code up the
   stack, in which case somebody eventually looks at errno.
   Handling these with a helper would be hard. Ideally there
   would be some errno value that we could set for short
   read, but there's no standard one (ENODATA is kind-of
   accurate, but it's not really "no data"; it's "not enough
   data", and we wouldn't want to get confused with a real
   ENODATA error).
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/get-tar-commit-id.c |  3 +--
 bulk-checkin.c              |  4 +---
 cache.h                     |  7 +++++++
 combine-diff.c              |  8 +-------
 csum-file.c                 |  6 +-----
 pack-write.c                |  3 +--
 wrapper.c                   | 11 +++++++++++
 7 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index 259ad40339..0b97196afc 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -24,8 +24,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)
 	if (argc != 1)
 		usage(builtin_get_tar_commit_id_usage);
 
-	if (read_in_full(0, buffer, HEADERSIZE) != HEADERSIZE)
-		die_errno("git get-tar-commit-id: read error");
+	xread_in_full(0, buffer, HEADERSIZE, "stdin");
 	if (header->typeflag[0] != 'g')
 		return 1;
 	if (!skip_prefix(content, "52 comment=", &comment))
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 9a1f6c49ab..a9743785fb 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -115,9 +115,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 
 		if (size && !s.avail_in) {
 			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
-			if (read_in_full(fd, ibuf, rsize) != rsize)
-				die("failed to read %d bytes from '%s'",
-				    (int)rsize, path);
+			xread_in_full(fd, ibuf, rsize, path);
 			offset += rsize;
 			if (*already_hashed_to < offset) {
 				size_t hsize = offset - *already_hashed_to;
diff --git a/cache.h b/cache.h
index 49b083ee0a..deec532228 100644
--- a/cache.h
+++ b/cache.h
@@ -1757,6 +1757,13 @@ extern ssize_t read_in_full(int fd, void *buf, size_t count);
 extern ssize_t write_in_full(int fd, const void *buf, size_t count);
 extern ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);
 
+/*
+ * Like read_in_full(), but die on errors or a failure to read exactly "count"
+ * bytes. The "desc" string will be inserted into the error message as "reading
+ * %s".
+ */
+extern void xread_in_full(int fd, void *buf, size_t count, const char *desc);
+
 static inline ssize_t write_str_in_full(int fd, const char *str)
 {
 	return write_in_full(fd, str, strlen(str));
diff --git a/combine-diff.c b/combine-diff.c
index 9e163d5ada..4bdf797a10 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1027,7 +1027,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			free_filespec(df);
 		} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
 			size_t len = xsize_t(st.st_size);
-			ssize_t done;
 			int is_file, i;
 
 			elem->mode = canon_mode(st.st_mode);
@@ -1042,12 +1041,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 
 			result_size = len;
 			result = xmallocz(len);
-
-			done = read_in_full(fd, result, len);
-			if (done < 0)
-				die_errno("read error '%s'", elem->path);
-			else if (done < len)
-				die("early EOF '%s'", elem->path);
+			xread_in_full(fd, result, len, elem->path);
 
 			/* If not a fake symlink, apply filters, e.g. autocrlf */
 			if (is_file) {
diff --git a/csum-file.c b/csum-file.c
index a172199e44..8a9a6075a4 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -15,12 +15,8 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)
 {
 	if (0 <= f->check_fd && count)  {
 		unsigned char check_buffer[8192];
-		ssize_t ret = read_in_full(f->check_fd, check_buffer, count);
 
-		if (ret < 0)
-			die_errno("%s: sha1 file read error", f->name);
-		if (ret < count)
-			die("%s: sha1 file truncated", f->name);
+		xread_in_full(f->check_fd, check_buffer, count, f->name);
 		if (memcmp(buf, check_buffer, count))
 			die("sha1 file '%s' validation error", f->name);
 	}
diff --git a/pack-write.c b/pack-write.c
index a333ec6754..05a7defdd2 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -219,8 +219,7 @@ void fixup_pack_header_footer(int pack_fd,
 
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
-	if (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
-		die_errno("Unable to reread header of '%s'", pack_name);
+	xread_in_full(pack_fd, &hdr, sizeof(hdr), pack_name);
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
 	git_SHA1_Update(&old_sha1_ctx, &hdr, sizeof(hdr));
diff --git a/wrapper.c b/wrapper.c
index f55debc92d..66ba4dbff3 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -329,6 +329,17 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
 	return total;
 }
 
+void xread_in_full(int fd, void *buf, size_t count, const char *desc)
+{
+	ssize_t ret = read_in_full(fd, buf, count);
+
+	if (ret < 0)
+		die_errno("error reading %s", desc);
+	if (ret != count)
+		die("too few bytes while reading %s (expected %"PRIuMAX", got %"PRIuMAX")",
+		    desc, (uintmax_t)count, (uintmax_t)ret);
+}
+
 ssize_t write_in_full(int fd, const void *buf, size_t count)
 {
 	const char *p = buf;

-- 
2.14.1.1148.ga2561536a1
Previous: Jeff KingNext: Jonathan Nieder
Message 37 of 51 in “read/write_in_full leftovers”
  1. 0/7 read/write_in_full leftoversJeff King, Sep 25, 2017
  2. 1/7 files-backend: prefer "0" for write_in_full() error checkJeff King, Sep 25, 2017
  3. Jonathan NiederSep 25, 2017
  4. Jeff KingSep 25, 2017
  5. 2/7 notes-merge: drop dead zero-write codeJeff King, Sep 25, 2017
  6. Jonathan NiederSep 25, 2017
  7. 3/7 read_in_full: reset errno before readingJeff King, Sep 25, 2017
  8. Jonathan NiederSep 25, 2017
  9. Jeff KingSep 25, 2017
  10. Jonathan NiederSep 25, 2017
  11. Jeff KingSep 25, 2017
  12. Jeff KingSep 25, 2017
  13. Jonathan NiederSep 25, 2017
  14. Jeff KingSep 26, 2017
  15. Stefan BellerSep 26, 2017
  16. Jeff KingSep 26, 2017
  17. Jonathan NiederSep 26, 2017
  18. Jonathan NiederSep 26, 2017
  19. Jeff KingSep 26, 2017
  20. Jonathan NiederSep 26, 2017
  21. Junio C HamanoSep 26, 2017
  22. Jeff KingSep 27, 2017
  23. Junio C HamanoSep 27, 2017
  24. Jonathan NiederSep 26, 2017
  25. Jeff KingSep 26, 2017
  26. Jonathan NiederSep 26, 2017
  27. Jeff KingSep 26, 2017
  28. Jonathan NiederSep 26, 2017
  29. Jonathan NiederSep 25, 2017
  30. 4/7 get-tar-commit-id: prefer "!=" for read_in_full() error checkJeff King, Sep 25, 2017
  31. Jonathan NiederSep 25, 2017
  32. 5/7 worktree: use xsize_t to access file sizeJeff King, Sep 25, 2017
  33. Jonathan NiederSep 25, 2017
  34. 6/7 worktree: check the result of read_in_full()Jeff King, Sep 25, 2017
  35. Jonathan NiederSep 25, 2017
  36. Jeff KingSep 25, 2017
  37. 7/7 add xread_in_full() helperJeff King, Sep 25, 2017
  38. Jonathan NiederSep 25, 2017
  39. Jeff KingSep 25, 2017
  40. 0/7 read/write_in_full leftoversJeff King, Sep 27, 2017
  41. 1/7 files-backend: prefer "0" for write_in_full() error checkJeff King, Sep 27, 2017
  42. 2/7 notes-merge: drop dead zero-write codeJeff King, Sep 27, 2017
  43. 3/7 prefer "!=" when checking read_in_full() resultJeff King, Sep 27, 2017
  44. Junio C HamanoSep 27, 2017
  45. Jeff KingSep 27, 2017
  46. 4/7 avoid looking at errno for short read_in_full() returnsJeff King, Sep 27, 2017
  47. Junio C HamanoSep 27, 2017
  48. 5/7 distinguish error versus short read from read_in_full()Jeff King, Sep 27, 2017
  49. 6/7 worktree: use xsize_t to access file sizeJeff King, Sep 27, 2017
  50. 7/7 worktree: check the result of read_in_full()Jeff King, Sep 27, 2017
  51. Junio C HamanoSep 27, 2017

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.