{"thread":{"id":"46832","subject":"[PATCH 0/7] read/write_in_full leftovers","startedAt":"2017-09-25T20:26:53Z","lastAt":"2017-09-27T07:10:06Z","messageCount":51,"participants":["Jeff King","Jonathan Nieder","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"328873","messageId":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","threadId":"46832","inReplyTo":null,"subject":"[PATCH 0/7] read/write_in_full leftovers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:26:47Z","receivedAt":"2017-09-25T20:26:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series addresses the bits leftover from the discussion two weeks\nago in:\n\n https://public-inbox.org/git/20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net/\n\nand its subthread. I don't think any of these is a real problem in\npractice, so this can be considered as a cleanup. I'm on the fence over\nwhether the final one is a good idea. See the commit message for\ndetails.\n\n [1/7]: files-backend: prefer \"0\" for write_in_full() error check\n [2/7]: notes-merge: drop dead zero-write code\n [3/7]: read_in_full: reset errno before reading\n [4/7]: get-tar-commit-id: prefer \"!=\" for read_in_full() error check\n [5/7]: worktree: use xsize_t to access file size\n [6/7]: worktree: check the result of read_in_full()\n [7/7]: add xread_in_full() helper\n\n builtin/get-tar-commit-id.c | 5 +----\n builtin/worktree.c | 11 ++++++++---\n bulk-checkin.c | 4 +---\n cache.h | 7 +++++++\n combine-diff.c | 8 +-------\n csum-file.c | 6 +-----\n notes-merge.c | 2 --\n pack-write.c | 3 +--\n refs/files-backend.c | 2 +-\n wrapper.c | 12 ++++++++++++\n 10 files changed, 33 insertions(+), 27 deletions(-)\n\n-Peff\n"},{"id":"328874","messageId":"20170925202716.g2wph3ensmzerm44@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH 1/7] files-backend: prefer \"0\" for write_in_full() error check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:27:17Z","receivedAt":"2017-09-25T20:27:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 06f46f237a (avoid \"write_in_full(fd, buf, len) !=\nlen\" pattern, 2017-09-13) converted this callsite from:\n\n write_in_full(...) != 1\n\nto\n\n write_in_full(...) < 0\n\nBut during the conflict resolution in c50424a6f0 (Merge\nbranch 'jk/write-in-full-fix', 2017-09-25), this morphed\ninto\n\n write_in_full(...) < 1\n\nThis behaves as we want, but we prefer to avoid modeling the\n\"less than length\" error-check which can be subtly buggy, as\nshown in efacf609c8 (config: avoid \"write_in_full(fd, buf,\nlen) < len\" pattern, 2017-09-13).\n\nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex dac33628b3..e35c64c001 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3007,7 +3007,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t} else if (update &&\n \t\t\t (write_in_full(get_lock_file_fd(&lock->lk),\n \t\t\t\toid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||\n-\t\t\t write_str_in_full(get_lock_file_fd(&lock->lk), \"\\n\") < 1 ||\n+\t\t\t write_str_in_full(get_lock_file_fd(&lock->lk), \"\\n\") < 0 ||\n \t\t\t close_ref_gently(lock) < 0)) {\n \t\t\tstatus |= error(\"couldn't write %s\",\n \t\t\t\t\tget_lock_file_path(&lock->lk));\n-- \n2.14.1.1148.ga2561536a1\n\n"},{"id":"328875","messageId":"20170925202757.w6eb556kq3ecyks3@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH 2/7] notes-merge: drop dead zero-write code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:27:57Z","receivedAt":"2017-09-25T20:28:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We call write_in_full() with a size that we know is greater\nthan zero. The return value can never be zero, then, since\nwrite_in_full() converts such a failed write() into ENOSPC\nand returns -1. We can just drop this branch of the error\nhandling entirely.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Jeff King \n---\n notes-merge.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 597d43f65c..4352c34a6e 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -308,8 +308,6 @@ static void write_buf_to_worktree(const struct object_id *obj,\n \t\t\tif (errno == EPIPE)\n \t\t\t\tbreak;\n \t\t\tdie_errno(\"notes-merge\");\n-\t\t} else if (!ret) {\n-\t\t\tdie(\"notes-merge: disk full?\");\n \t\t}\n \t\tsize -= ret;\n \t\tbuf += ret;\n-- \n2.14.1.1148.ga2561536a1\n\n"},{"id":"328876","messageId":"20170925202916.4tqo4gttrsoy7kai@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:29:16Z","receivedAt":"2017-09-25T20:29:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many callers of read_in_full() complain when we do not read\ntheir full byte-count. But a check like:\n\n if (read_in_full(fd, buf, len) != len)\n\t return error_errno(\"unable to read\");\n\nconflates two problem conditions:\n\n 1. A real error from read().\n\n 2. There were fewer than \"len\" bytes available.\n\nIn the first case, showing the user strerror(errno) is\nuseful. But in the second, we may see a random errno that\nwas set by some previous system call.\n\nIn an ideal world, callers would always distinguish between\nthese cases and give a useful message for each. But as an\neasy way to make our imperfect world better, let's reset\nerrno to a known value. The best we can do is \"0\", which\nwill yield something like:\n\n unable to read: Success\n\nThat's not great, but at least it's deterministic and makes\nit clear that we didn't see an error from read().\n\nSigned-off-by: Jeff King \n---\n wrapper.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 61aba0b5c1..f55debc92d 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -314,6 +314,7 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n \tchar *p = buf;\n \tssize_t total = 0;\n \n+\terrno = 0;\n \twhile (count > 0) {\n \t\tssize_t loaded = xread(fd, p, count);\n \t\tif (loaded < 0)\n-- \n2.14.1.1148.ga2561536a1\n\n"},{"id":"328877","messageId":"20170925202947.u6onp66ztliyxjcj@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH 4/7] get-tar-commit-id: prefer \"!=\" for read_in_full() error check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:29:48Z","receivedAt":"2017-09-25T20:29:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Comparing the result of read_in_full() using less-than is\npotentially dangerous, as discussed in 561598cfcf\n(read_pack_header: handle signed/unsigned comparison in read\nresult, 2017-09-13).\n\nThe instance in get-tar-commit-id is OK, because the\nHEADERSIZE macro expands to a signed integer. But if it were\nswitched to an unsigned type like:\n\n size_t HEADERSIZE = ...;\n\nthis would be a bug. Let's use the more robust \"!=\"\nconstruct.\n\nWe can also drop the useless \"n\" variable while we're at it.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Jeff King \n---\n builtin/get-tar-commit-id.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 6d9a79f9b3..259ad40339 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -20,14 +20,12 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \tstruct ustar_header *header = (struct ustar_header *)buffer;\n \tchar *content = buffer + RECORDSIZE;\n \tconst char *comment;\n-\tssize_t n;\n \n \tif (argc != 1)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \n-\tn = read_in_full(0, buffer, HEADERSIZE);\n-\tif (n < HEADERSIZE)\n-\t\tdie(\"git get-tar-commit-id: read error\");\n+\tif (read_in_full(0, buffer, HEADERSIZE) != HEADERSIZE)\n+\t\tdie_errno(\"git get-tar-commit-id: read error\");\n \tif (header->typeflag[0] != 'g')\n \t\treturn 1;\n \tif (!skip_prefix(content, \"52 comment=\", &comment))\n-- \n2.14.1.1148.ga2561536a1\n\n"},{"id":"328878","messageId":"20170925203033.z4czmuhedm3f54b3@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH 5/7] worktree: use xsize_t to access file size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:30:34Z","receivedAt":"2017-09-25T20:30:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To read the \"gitdir\" file into memory, we stat the file and\nallocate a buffer. But we store the size in an \"int\", which\nmay be truncated. We should use a size_t and xsize_t(),\nwhich will detect truncation.\n\nAn overflow is unlikely for a \"gitdir\" file, but it's a good\npractice to model.\n\nSigned-off-by: Jeff King \n---\n builtin/worktree.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex de26849f55..2f4a4ef9cd 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -38,7 +38,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n {\n \tstruct stat st;\n \tchar *path;\n-\tint fd, len;\n+\tint fd;\n+\tsize_t len;\n \n \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n \t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: not a valid directory\"), id);\n@@ -56,7 +57,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \t\t\t id, strerror(errno));\n \t\treturn 1;\n \t}\n-\tlen = st.st_size;\n+\tlen = xsize_t(st.st_size);\n \tpath = xmallocz(len);\n \tread_in_full(fd, path, len);\n \tclose(fd);\n-- \n2.14.1.1148.ga2561536a1\n\n"},{"id":"328879","messageId":"20170925203156.boieic627t3dbpzd@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH 6/7] worktree: check the result of read_in_full()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:31:56Z","receivedAt":"2017-09-25T20:32:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We try to read \"len\" bytes into a buffer and just assume\nthat it happened correctly. In practice this should usually\nbe the case, since we just stat'd the file to get the\nlength. But we could be fooled by transient errors or by\nother processes racily truncating the file.\n\nLet's be more careful. There's a slim chance this could\ncatch a real error, but it also prevents people and tools\nfrom getting worried while reading the code.\n\nSigned-off-by: Jeff King \n---\n builtin/worktree.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 2f4a4ef9cd..87b3d70b0b 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -59,7 +59,11 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \t}\n \tlen = xsize_t(st.st_size);\n \tpath = xmallocz(len);\n-\tread_in_full(fd, path, len);\n+\tif (read_in_full(fd, path, len) != len) {\n+\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir read did not match stat (%s)\"),\n+\t\t\t id, strerror(errno));\n+\t\treturn 1;\n+\t}\n \tclose(fd);\n \twhile (len && (path[len - 1] == '\\n' || path[len - 1] == '\\r'))\n \t\tlen--;\n-- \n2.14.1.1148.ga2561536a1\n\n"},{"id":"328880","messageId":"20170925203323.rmm3i5kx266ma3wu@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH 7/7] add xread_in_full() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T20:33:24Z","receivedAt":"2017-09-25T20:33:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many callers of read_in_full() expect to see exactly \"len\"\nbytes, and die if that isn't the case. Since there are two\ninteresting error cases:\n\n 1. We saw a read() error.\n\n 2. We saw EOF with fewer bytes than expected.\n\nwe would ideally distinguish between them when reporting to\nthe user. Let's add a helper to make this easier. We can\nconvert cases that currently lump the two conditions\ntogether (improving their error messages) and ones that\nalready distinguish (shortening their code).\n\nThere are a number of read_in_full() calls left that we\ncan't (or shouldn't) convert, for one or more of these\nreasons:\n\n - Some are not trying to read an exact number in the first\n place. So they only care about case 1, and are fine.\n\n - Some call error() instead of die(). We could possibly\n accommodate these, at the expense of making our helper a\n bit more clunky.\n\n - Some print nothing and return a failing code up the\n stack, in which case somebody eventually looks at errno.\n Handling these with a helper would be hard. Ideally there\n would be some errno value that we could set for short\n read, but there's no standard one (ENODATA is kind-of\n accurate, but it's not really \"no data\"; it's \"not enough\n data\", and we wouldn't want to get confused with a real\n ENODATA error).\n\nSigned-off-by: Jeff King \n---\n builtin/get-tar-commit-id.c | 3 +--\n bulk-checkin.c | 4 +---\n cache.h | 7 +++++++\n combine-diff.c | 8 +-------\n csum-file.c | 6 +-----\n pack-write.c | 3 +--\n wrapper.c | 11 +++++++++++\n 7 files changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 259ad40339..0b97196afc 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -24,8 +24,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \tif (argc != 1)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \n-\tif (read_in_full(0, buffer, HEADERSIZE) != HEADERSIZE)\n-\t\tdie_errno(\"git get-tar-commit-id: read error\");\n+\txread_in_full(0, buffer, HEADERSIZE, \"stdin\");\n \tif (header->typeflag[0] != 'g')\n \t\treturn 1;\n \tif (!skip_prefix(content, \"52 comment=\", &comment))\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 9a1f6c49ab..a9743785fb 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -115,9 +115,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,\n \n \t\tif (size && !s.avail_in) {\n \t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n-\t\t\tif (read_in_full(fd, ibuf, rsize) != rsize)\n-\t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n-\t\t\t\t (int)rsize, path);\n+\t\t\txread_in_full(fd, ibuf, rsize, path);\n \t\t\toffset += rsize;\n \t\t\tif (*already_hashed_to < offset) {\n \t\t\t\tsize_t hsize = offset - *already_hashed_to;\ndiff --git a/cache.h b/cache.h\nindex 49b083ee0a..deec532228 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1757,6 +1757,13 @@ extern ssize_t read_in_full(int fd, void *buf, size_t count);\n extern ssize_t write_in_full(int fd, const void *buf, size_t count);\n extern ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);\n \n+/*\n+ * Like read_in_full(), but die on errors or a failure to read exactly \"count\"\n+ * bytes. The \"desc\" string will be inserted into the error message as \"reading\n+ * %s\".\n+ */\n+extern void xread_in_full(int fd, void *buf, size_t count, const char *desc);\n+\n static inline ssize_t write_str_in_full(int fd, const char *str)\n {\n \treturn write_in_full(fd, str, strlen(str));\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 9e163d5ada..4bdf797a10 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1027,7 +1027,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tfree_filespec(df);\n \t\t} else if (0 <= (fd = open(elem->path, O_RDONLY))) {\n \t\t\tsize_t len = xsize_t(st.st_size);\n-\t\t\tssize_t done;\n \t\t\tint is_file, i;\n \n \t\t\telem->mode = canon_mode(st.st_mode);\n@@ -1042,12 +1041,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \n \t\t\tresult_size = len;\n \t\t\tresult = xmallocz(len);\n-\n-\t\t\tdone = read_in_full(fd, result, len);\n-\t\t\tif (done < 0)\n-\t\t\t\tdie_errno(\"read error '%s'\", elem->path);\n-\t\t\telse if (done < len)\n-\t\t\t\tdie(\"early EOF '%s'\", elem->path);\n+\t\t\txread_in_full(fd, result, len, elem->path);\n \n \t\t\t/* If not a fake symlink, apply filters, e.g. autocrlf */\n \t\t\tif (is_file) {\ndiff --git a/csum-file.c b/csum-file.c\nindex a172199e44..8a9a6075a4 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -15,12 +15,8 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)\n {\n \tif (0 <= f->check_fd && count) {\n \t\tunsigned char check_buffer[8192];\n-\t\tssize_t ret = read_in_full(f->check_fd, check_buffer, count);\n \n-\t\tif (ret < 0)\n-\t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n-\t\tif (ret < count)\n-\t\t\tdie(\"%s: sha1 file truncated\", f->name);\n+\t\txread_in_full(f->check_fd, check_buffer, count, f->name);\n \t\tif (memcmp(buf, check_buffer, count))\n \t\t\tdie(\"sha1 file '%s' validation error\", f->name);\n \t}\ndiff --git a/pack-write.c b/pack-write.c\nindex a333ec6754..05a7defdd2 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -219,8 +219,7 @@ void fixup_pack_header_footer(int pack_fd,\n \n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n \t\tdie_errno(\"Failed seeking to start of '%s'\", pack_name);\n-\tif (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n-\t\tdie_errno(\"Unable to reread header of '%s'\", pack_name);\n+\txread_in_full(pack_fd, &hdr, sizeof(hdr), pack_name);\n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n \t\tdie_errno(\"Failed seeking to start of '%s'\", pack_name);\n \tgit_SHA1_Update(&old_sha1_ctx, &hdr, sizeof(hdr));\ndiff --git a/wrapper.c b/wrapper.c\nindex f55debc92d..66ba4dbff3 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -329,6 +329,17 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n \treturn total;\n }\n \n+void xread_in_full(int fd, void *buf, size_t count, const char *desc)\n+{\n+\tssize_t ret = read_in_full(fd, buf, count);\n+\n+\tif (ret < 0)\n+\t\tdie_errno(\"error reading %s\", desc);\n+\tif (ret != count)\n+\t\tdie(\"too few bytes while reading %s (expected %\"PRIuMAX\", got %\"PRIuMAX\")\",\n+\t\t desc, (uintmax_t)count, (uintmax_t)ret);\n+}\n+\n ssize_t write_in_full(int fd, const void *buf, size_t count)\n {\n \tconst char *p = buf;\n-- \n2.14.1.1148.ga2561536a1\n"},{"id":"328882","messageId":"20170925215927.GZ27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925202716.g2wph3ensmzerm44@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] files-backend: prefer \"0\" for write_in_full() error check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T21:59:27Z","receivedAt":"2017-09-25T21:59:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> Commit 06f46f237a (avoid \"write_in_full(fd, buf, len) !=\n> len\" pattern, 2017-09-13) converted this callsite from:\n>\n> write_in_full(...) != 1\n>\n> to\n>\n> write_in_full(...) < 0\n>\n> But during the conflict resolution in c50424a6f0 (Merge\n> branch 'jk/write-in-full-fix', 2017-09-25), this morphed\n> into\n>\n> write_in_full(...) < 1\n>\n> This behaves as we want, but we prefer to avoid modeling the\n> \"less than length\" error-check which can be subtly buggy, as\n> shown in efacf609c8 (config: avoid \"write_in_full(fd, buf,\n> len) < len\" pattern, 2017-09-13).\n>\n> Signed-off-by: Jeff King \n> ---\n> refs/files-backend.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nGood eyes. Thanks.\nReviewed-by: Jonathan Nieder \n\nBy the way, the description from that merge commit\n\n Many codepaths did not diagnose write failures correctly when disks\n go full, due to their misuse of write_in_full() helper function,\n which have been corrected.\n\ndoes not look accurate to me. At least the \"Many codepaths\" part.\nAll of those changes except for three should be no-ops. The scariest\none is the 'long ret = write_in_full(fd, buf, size)' in notes-merge.c,\nwhich is about overflowing a \"long\" on Windows instead of error\nhandling.\n\nThanks,\nJonathan\n"},{"id":"328883","messageId":"20170925215958.GA27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925202757.w6eb556kq3ecyks3@sigill.intra.peff.net","subject":"Re: [PATCH 2/7] notes-merge: drop dead zero-write code","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T21:59:58Z","receivedAt":"2017-09-25T22:00:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We call write_in_full() with a size that we know is greater\n> than zero. The return value can never be zero, then, since\n> write_in_full() converts such a failed write() into ENOSPC\n> and returns -1. We can just drop this branch of the error\n> handling entirely.\n>\n> Suggested-by: Jonathan Nieder \n> Signed-off-by: Jeff King \n> ---\n> notes-merge.c | 2 --\n> 1 file changed, 2 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks for tying up this loose end.\n"},{"id":"328884","messageId":"20170925220914.GB27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925202916.4tqo4gttrsoy7kai@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T22:09:14Z","receivedAt":"2017-09-25T22:09:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> In an ideal world, callers would always distinguish between\n> these cases and give a useful message for each. But as an\n> easy way to make our imperfect world better, let's reset\n> errno to a known value. The best we can do is \"0\", which\n> will yield something like:\n>\n> unable to read: Success\n>\n> That's not great, but at least it's deterministic and makes\n> it clear that we didn't see an error from read().\n\nYuck. Can we set errno to something more specific instead?\n\nread(2) also doesn't promise not to clobber errno on success.\n\nHow about something like the following?\n\n-- >8 --\nSubject: read_in_full: set errno for partial reads\n\nMany callers of read_in_full() complain when we do not read their full\nbyte-count. But a check like:\n\n if (read_in_full(fd, buf, len) != len)\n\treturn error_errno(\"unable to read\");\n\nconflates two problem conditions:\n\n 1. A real error from read().\n 2. There were fewer than \"len\" bytes available.\n\nIn the first case, showing the user strerror(errno) is useful. In the\nsecond, we may see a random errno that was set by some previous system\ncall.\n\nIn an ideal world, callers would always distinguish between these\ncases and give a useful message for each. But as an easy way to make\nour imperfect world better, let's set errno in the second case to a\nknown value. There is no standard \"Unexpected end of file\" errno\nvalue, so instead use EILSEQ, which yields a message like\n\n unable to read: Illegal byte sequence\n\nThat's not great, but at least it's deterministic and makes it\npossible to reverse-engineer what went wrong.\n\nChange-Id: I48138052f71b7c6cf35c2d00a10dc8febaca4f10\nSigned-off-by: Jonathan Nieder \n---\n wrapper.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 61aba0b5c1..1842a99b87 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -318,8 +318,10 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n \t\tssize_t loaded = xread(fd, p, count);\n \t\tif (loaded < 0)\n \t\t\treturn -1;\n-\t\tif (loaded == 0)\n+\t\tif (loaded == 0) {\n+\t\t\terrno = EILSEQ;\n \t\t\treturn total;\n+\t\t}\n \t\tcount -= loaded;\n \t\tp += loaded;\n \t\ttotal += loaded;\n-- \n2.14.1.821.g8fa685d3b7\n\n"},{"id":"328885","messageId":"20170925221005.GC27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925202947.u6onp66ztliyxjcj@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] get-tar-commit-id: prefer \"!=\" for read_in_full() error check","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T22:10:05Z","receivedAt":"2017-09-25T22:10:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Suggested-by: Jonathan Nieder \n> Signed-off-by: Jeff King \n> ---\n> builtin/get-tar-commit-id.c | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"328886","messageId":"20170925221101.GD27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925203033.z4czmuhedm3f54b3@sigill.intra.peff.net","subject":"Re: [PATCH 5/7] worktree: use xsize_t to access file size","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T22:11:01Z","receivedAt":"2017-09-25T22:11:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> To read the \"gitdir\" file into memory, we stat the file and\n> allocate a buffer. But we store the size in an \"int\", which\n> may be truncated. We should use a size_t and xsize_t(),\n> which will detect truncation.\n>\n> An overflow is unlikely for a \"gitdir\" file, but it's a good\n> practice to model.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/worktree.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"328887","messageId":"20170925221426.GE27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925203156.boieic627t3dbpzd@sigill.intra.peff.net","subject":"Re: [PATCH 6/7] worktree: check the result of read_in_full()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T22:14:26Z","receivedAt":"2017-09-25T22:14:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We try to read \"len\" bytes into a buffer and just assume\n> that it happened correctly. In practice this should usually\n> be the case, since we just stat'd the file to get the\n> length. But we could be fooled by transient errors or by\n> other processes racily truncating the file.\n>\n> Let's be more careful. There's a slim chance this could\n> catch a real error, but it also prevents people and tools\n> from getting worried while reading the code.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/worktree.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 2f4a4ef9cd..87b3d70b0b 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -59,7 +59,11 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n> \t}\n> \tlen = xsize_t(st.st_size);\n> \tpath = xmallocz(len);\n> -\tread_in_full(fd, path, len);\n> +\tif (read_in_full(fd, path, len) != len) {\n> +\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir read did not match stat (%s)\"),\n> +\t\t\t id, strerror(errno));\n\nI'm a little confused. The 'if' condition checks for a read error but\nthe message says something about 'stat'.\n\nIf we're trying to double-check the 'stat' result, shouldn't we read\nall the way to EOF in case the file got longer?\n\nPuzzled,\nJonathan\n"},{"id":"328888","messageId":"20170925221608.GF27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925203323.rmm3i5kx266ma3wu@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] add xread_in_full() helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T22:16:08Z","receivedAt":"2017-09-25T22:16:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Many callers of read_in_full() expect to see exactly \"len\"\n> bytes, and die if that isn't the case.\n\nmicronit: Can this be named read_in_full_or_die?\n\nOtherwise it's too easy to mistake for a function like xread, which\nhas different semantics.\n\nI realize that xmalloc, xmemdupz, etc use a different convention.\nThat's yet another reason to be explicit, IMHO.\n\nThanks,\nJonathan\n"},{"id":"328894","messageId":"20170925231308.hpk7xrelqyqy6clg@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925215927.GZ27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/7] files-backend: prefer \"0\" for write_in_full() error check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T23:13:08Z","receivedAt":"2017-09-25T23:13:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 02:59:27PM -0700, Jonathan Nieder wrote:\n\n> > But during the conflict resolution in c50424a6f0 (Merge\n> > branch 'jk/write-in-full-fix', 2017-09-25), this morphed\n> > into\n> [...]\n> Good eyes. Thanks.\n\nSort of. :) I usually continually rebase my topics until they end up\nempty. So I notice bits like this either during conflict resolution, or\nwhen the topic is left unexpectedly non-empty.\n\nIt's not foolproof, though. Sometimes rebasing a topic on itself is so\npainful that I \"rebase --skip\" liberally in the early parts, and would\nmiss any merge funniness.\n\n> By the way, the description from that merge commit\n> \n> Many codepaths did not diagnose write failures correctly when disks\n> go full, due to their misuse of write_in_full() helper function,\n> which have been corrected.\n> \n> does not look accurate to me. At least the \"Many codepaths\" part.\n> All of those changes except for three should be no-ops. The scariest\n> one is the 'long ret = write_in_full(fd, buf, size)' in notes-merge.c,\n> which is about overflowing a \"long\" on Windows instead of error\n> handling.\n\nThe ones in config.c are definitely wrong, too. Though I'd agree that\n\"many\" might be overstating it.\n\n-Peff\n"},{"id":"328895","messageId":"20170925232313.hhtqpxuzpzbnbdop@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925220914.GB27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T23:23:13Z","receivedAt":"2017-09-25T23:23:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 03:09:14PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > In an ideal world, callers would always distinguish between\n> > these cases and give a useful message for each. But as an\n> > easy way to make our imperfect world better, let's reset\n> > errno to a known value. The best we can do is \"0\", which\n> > will yield something like:\n> >\n> > unable to read: Success\n> >\n> > That's not great, but at least it's deterministic and makes\n> > it clear that we didn't see an error from read().\n> \n> Yuck. Can we set errno to something more specific instead?\n\nYes, but what? You've suggested EILSEQ here, but that feels a\nbit...funny. I guess at least it's unlikely that read() would ever set\nerrno to that itself, so we can be reasonably sure that seeing it is a\nsign of a short read. I thought ERANGE might be a possible candidate,\nbut it doesn't quite fit. Ditto for ENODATA.\n\nI _would_ like to have something meaningful, as it would mean the whole\nxread_in_full() nonsense from the final patch would be unnecessary. It\nwould simply be reasonable to show the errno after read_in_full.\n\nAnother question is whether EILSEQ (or the others) is actually defined\neverywhere we compile. If it isn't, we'll have to define it ourselves\n(but to what? strerror() won't return anything useful for it).\n\n> read(2) also doesn't promise not to clobber errno on success.\n\nYes, though it's only a problem if you're using something other than 0.\n\nSpeaking of funny errno clobbering, with your patch:\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 61aba0b5c1..1842a99b87 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -318,8 +318,10 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n> \t\tssize_t loaded = xread(fd, p, count);\n> \t\tif (loaded < 0)\n> \t\t\treturn -1;\n> -\t\tif (loaded == 0)\n> +\t\tif (loaded == 0) {\n> +\t\t\terrno = EILSEQ;\n> \t\t\treturn total;\n> +\t\t}\n\nIf I do this:\n\n errno = 0;\n read_in_full(fd, buf, sizeof(buf));\n if (errno)\n\tdie_errno(\"oops\");\n\nthen we'll claim an error, even though there was none (remember that\nit's only an error for _some_ callers to not read the whole length).\n\nThis may be sufficiently odd that we don't need to care about it. There\nare some calls (like strtoul) which require this kind of clear-and-check\nstrategy with errno, but in general we frown on it for calls like\nread().\n\nWe could also introduce a better helper like this:\n\n int read_exactly(int fd, void *buf, size_t count)\n {\n\tssize_t ret = read_in_full(fd, buf, count);\n\tif (ret < 0)\n\t\treturn -1;\n\tif (ret != count) {\n\t\terrno = EILSEQ;\n\t\treturn -1;\n\t}\n\treturn 0;\n }\n\nThen we know that touching errno always coincides with an error return.\nAnd it's shorter to check \"< 0\" compared to \"!= count\" in the caller.\nBut of course a caller which wants to distinguish the two cases for its\nerror messages then has to look at errno:\n\n if (read_exactly(fd, buf, len) < 0) {\n\tif (errno == EILSEQ) /* eek, now this abstraction is leaky */\n\t\tdie(\"short read\");\n\telse\n\t\tdie_errno(\"read error\");\n }\n\nI dunno. All options seem pretty gross to me.\n\n-Peff\n"},{"id":"328896","messageId":"20170925232519.ljbt37r7qvb4or32@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925221426.GE27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 6/7] worktree: check the result of read_in_full()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T23:25:19Z","receivedAt":"2017-09-25T23:25:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 03:14:26PM -0700, Jonathan Nieder wrote:\n\n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > index 2f4a4ef9cd..87b3d70b0b 100644\n> > --- a/builtin/worktree.c\n> > +++ b/builtin/worktree.c\n> > @@ -59,7 +59,11 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n> > \t}\n> > \tlen = xsize_t(st.st_size);\n> > \tpath = xmallocz(len);\n> > -\tread_in_full(fd, path, len);\n> > +\tif (read_in_full(fd, path, len) != len) {\n> > +\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir read did not match stat (%s)\"),\n> > +\t\t\t id, strerror(errno));\n> \n> I'm a little confused. The 'if' condition checks for a read error but\n> the message says something about 'stat'.\n> \n> If we're trying to double-check the 'stat' result, shouldn't we read\n> all the way to EOF in case the file got longer?\n\nIf you wanted to really double-check the stat result, yes you'd want to\nmake sure there aren't extra bytes either. But really we're just making\nsure we were able to read _enough_ bytes.\n\nTo be honest, I'm tempted to rip out the whole thing and replace it with\nstrbuf_read_file(), which seems more straightforward.\n\nThe function does seem to rely on the stat() to get the mtime, so we'd\nhave to leave that part in.\n\n-Peff\n"},{"id":"328897","messageId":"20170925232706.ulrefbv623tinukf@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925221608.GF27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 7/7] add xread_in_full() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T23:27:06Z","receivedAt":"2017-09-25T23:27:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 03:16:08PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Many callers of read_in_full() expect to see exactly \"len\"\n> > bytes, and die if that isn't the case.\n> \n> micronit: Can this be named read_in_full_or_die?\n> \n> Otherwise it's too easy to mistake for a function like xread, which\n> has different semantics.\n> \n> I realize that xmalloc, xmemdupz, etc use a different convention.\n> That's yet another reason to be explicit, IMHO.\n\nYeah, we've definitely misused the \"x\" prefix for different things. I\nagree that being explicit probably can't hurt.\n\nI wonder if it's worth calling it \"read_exactly_or_die()\" to emphasize\nthat not reading enough bytes is a die-able offense.\n\n-Peff\n"},{"id":"328898","messageId":"20170925233316.GI27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925232313.hhtqpxuzpzbnbdop@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T23:33:16Z","receivedAt":"2017-09-25T23:33:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -318,8 +318,10 @@ ssize_t read_in_full(int fd, void *buf, size_t count)\n>> \t\tssize_t loaded = xread(fd, p, count);\n>> \t\tif (loaded < 0)\n>> \t\t\treturn -1;\n>> -\t\tif (loaded == 0)\n>> +\t\tif (loaded == 0) {\n>> +\t\t\terrno = EILSEQ;\n>> \t\t\treturn total;\n>> +\t\t}\n>\n> If I do this:\n>\n> errno = 0;\n> read_in_full(fd, buf, sizeof(buf));\n> if (errno)\n> \tdie_errno(\"oops\");\n>\n> then we'll claim an error, even though there was none (remember that\n> it's only an error for _some_ callers to not read the whole length).\n>\n> This may be sufficiently odd that we don't need to care about it. There\n> are some calls (like strtoul) which require this kind of clear-and-check\n> strategy with errno, but in general we frown on it for calls like\n> read().\n\nCorrect. Actually more than \"frown on\": except for with the few calls\nlike strtoul that are advertised to work this way, POSIX does not make\nthe guarantee the above code would rely on, at all.\n\nSo it's not just frowned upon: it's so unportable that the standard\ncalls it out as something that won't work.\n\n> We could also introduce a better helper like this:\n>\n> int read_exactly(int fd, void *buf, size_t count)\n> {\n> \tssize_t ret = read_in_full(fd, buf, count);\n> \tif (ret < 0)\n> \t\treturn -1;\n> \tif (ret != count) {\n> \t\terrno = EILSEQ;\n> \t\treturn -1;\n> \t}\n> \treturn 0;\n> }\n>\n> Then we know that touching errno always coincides with an error return.\n> And it's shorter to check \"< 0\" compared to \"!= count\" in the caller.\n> But of course a caller which wants to distinguish the two cases for its\n> error messages then has to look at errno:\n\nThat sounds nice, but it doesn't solve the original problem of callers\nusing read_in_full that way.\n\nThanks,\nJonathan\n"},{"id":"328899","messageId":"20170925233732.4lqavl56qwpfjuxy@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925233316.GI27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T23:37:33Z","receivedAt":"2017-09-25T23:37:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 04:33:16PM -0700, Jonathan Nieder wrote:\n\n> > If I do this:\n> >\n> > errno = 0;\n> > read_in_full(fd, buf, sizeof(buf));\n> > if (errno)\n> > \tdie_errno(\"oops\");\n> >\n> > then we'll claim an error, even though there was none (remember that\n> > it's only an error for _some_ callers to not read the whole length).\n> >\n> > This may be sufficiently odd that we don't need to care about it. There\n> > are some calls (like strtoul) which require this kind of clear-and-check\n> > strategy with errno, but in general we frown on it for calls like\n> > read().\n> \n> Correct. Actually more than \"frown on\": except for with the few calls\n> like strtoul that are advertised to work this way, POSIX does not make\n> the guarantee the above code would rely on, at all.\n> \n> So it's not just frowned upon: it's so unportable that the standard\n> calls it out as something that won't work.\n\nIs it unportable? Certainly read() is free reset errno to zero on\nsuccess. But is it allowed to set it to another random value?\n\nI think we're getting pretty academic here, but I'm curious if you have\na good reference.\n\n> > We could also introduce a better helper like this:\n> >\n> > int read_exactly(int fd, void *buf, size_t count)\n> > {\n> > \tssize_t ret = read_in_full(fd, buf, count);\n> > \tif (ret < 0)\n> > \t\treturn -1;\n> > \tif (ret != count) {\n> > \t\terrno = EILSEQ;\n> > \t\treturn -1;\n> > \t}\n> > \treturn 0;\n> > }\n> >\n> > Then we know that touching errno always coincides with an error return.\n> > And it's shorter to check \"< 0\" compared to \"!= count\" in the caller.\n> > But of course a caller which wants to distinguish the two cases for its\n> > error messages then has to look at errno:\n> \n> That sounds nice, but it doesn't solve the original problem of callers\n> using read_in_full that way.\n\nRight. None of the options discussed in this patch can do so. They can\nonly take a caller which doesn't distinguish between the cases, and give\nit a deterministic value in errno for the short-read case.\n\nIMHO as long as it _is_ deterministic and recognize as not an error from\nread(), that's the best we can do. Which is why I went with \"0\" in the\nfirst place. Seeing \"read error: success\" is a common-ish idiom (to me\nanyway) for \"read didn't fail, but some user-space logic did\", if only\nbecause it often happens accidentally.\n\n-Peff\n"},{"id":"328902","messageId":"20170925234541.l6qcislhfwtnxf5m@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925233732.4lqavl56qwpfjuxy@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T23:45:41Z","receivedAt":"2017-09-25T23:45:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 07:37:32PM -0400, Jeff King wrote:\n\n> > Correct. Actually more than \"frown on\": except for with the few calls\n> > like strtoul that are advertised to work this way, POSIX does not make\n> > the guarantee the above code would rely on, at all.\n> > \n> > So it's not just frowned upon: it's so unportable that the standard\n> > calls it out as something that won't work.\n> \n> Is it unportable? Certainly read() is free reset errno to zero on\n> success. But is it allowed to set it to another random value?\n> \n> I think we're getting pretty academic here, but I'm curious if you have\n> a good reference.\n\nAnswering my own question. POSIX says:\n\n No function in this volume of IEEE Std 1003.1-2001 shall set errno to\n 0. The setting of errno after a successful call to a function is\n unspecified unless the description of that function specifies that\n errno shall not be modified.\n\nSo that does seem to outlaw errno-only checks for most functions.\n\nIt makes me wonder if the recent getdelim() fix is technically violating\nthis. It should instead be explicitly checking for feof().\n\n> IMHO as long as it _is_ deterministic and recognize as not an error from\n> read(), that's the best we can do. Which is why I went with \"0\" in the\n> first place. Seeing \"read error: success\" is a common-ish idiom (to me\n> anyway) for \"read didn't fail, but some user-space logic did\", if only\n> because it often happens accidentally.\n\nAnother option I ran across from POSIX:\n\n [EOVERFLOW]\n The file is a regular file, nbyte is greater than 0, the starting\n position is before the end-of-file, and the starting position is\n greater than or equal to the offset maximum established in the open\n file description associated with fildes.\n\nThat's not _exactly_ what's going on here, but it's pretty close. And is\nwhat you would get if you implemented read_exactly() in terms of\nsomething like pread().\n\n-Peff\n"},{"id":"328903","messageId":"20170925235230.GK27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925233732.4lqavl56qwpfjuxy@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T23:52:30Z","receivedAt":"2017-09-25T23:52:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Sep 25, 2017 at 04:33:16PM -0700, Jonathan Nieder wrote:\n>> Jef King wrote:\n\n>>> errno = 0;\n>>> read_in_full(fd, buf, sizeof(buf));\n>>> if (errno)\n>>> \tdie_errno(\"oops\");\n[...]\n>>> in general we frown on it for calls like\n>>> read().\n>>\n>> Correct. Actually more than \"frown on\": except for with the few calls\n>> like strtoul that are advertised to work this way, POSIX does not make\n>> the guarantee the above code would rely on, at all.\n>>\n>> So it's not just frowned upon: it's so unportable that the standard\n>> calls it out as something that won't work.\n>\n> Is it unportable? Certainly read() is free reset errno to zero on\n> success. But is it allowed to set it to another random value?\n>\n> I think we're getting pretty academic here, but I'm curious if you have\n> a good reference.\n\nYes, it is allowed to set it to another random value. It's a common\nthing for implementations to do when they hit a recoverable error,\nwhich they sometimes do (e.g. think EFAULT, EINTR, ETIMEDOUT, or an\nimplementation calling strtoul and getting EINVAL or ERANGE).\n\nglibc only recently started trying to avoid this kind of behavior,\nbecause even though the standard allows it, users hate it.\n\nPOSIX.1-2008 XSI 2.3 \"Error Numbers\" says\n\n\tSome functions provide the error number in a variable accessed\n\tthrough the symbol errno, defined by including the \n\theader. The value of errno should only be examined when it is\n\tindicated to be valid by a function's return value.\n\nSee http://austingroupbugs.net/view.php?id=374 for an example where\nsomeone wanted to add a new guarantee of that kind to a function.\n\nHope that helps,\nJonathan\n"},{"id":"328904","messageId":"20170925235510.GL27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170925234541.l6qcislhfwtnxf5m@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-25T23:55:10Z","receivedAt":"2017-09-25T23:55:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> [EOVERFLOW]\n> The file is a regular file, nbyte is greater than 0, the starting\n> position is before the end-of-file, and the starting position is\n> greater than or equal to the offset maximum established in the open\n> file description associated with fildes.\n>\n> That's not _exactly_ what's going on here, but it's pretty close. And is\n> what you would get if you implemented read_exactly() in terms of\n> something like pread().\n\nAll that really matters is the strerror() that the user would see.\n\nFor EOVERFLOW, that is\n\n\tValue too large to be stored in data type\n\nFor EILSEQ, it is\n\n\tIllegal byte sequence\n\nNeither is perfect, but the latter seems like a better fit for the kind\nof file format errors we're describing here. Of course, it's even\nbetter if we fix the callers and don't try to wedge this into errno.\n\nIf you are okay with the too-long/too-short confusion in EOVERFLOW, then\nthere is EMSGSIZE:\n\n\tMessage too long\n\nHope that helps,\nJonathan\n"},{"id":"328905","messageId":"20170926000117.y3solltovyueq3zc@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925235510.GL27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-26T00:01:17Z","receivedAt":"2017-09-26T00:01:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 04:55:10PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > [EOVERFLOW]\n> > The file is a regular file, nbyte is greater than 0, the starting\n> > position is before the end-of-file, and the starting position is\n> > greater than or equal to the offset maximum established in the open\n> > file description associated with fildes.\n> >\n> > That's not _exactly_ what's going on here, but it's pretty close. And is\n> > what you would get if you implemented read_exactly() in terms of\n> > something like pread().\n> \n> All that really matters is the strerror() that the user would see.\n\nAgreed, though I didn't think those were necessarily standardized.\n\n> For EOVERFLOW, that is\n> \n> \tValue too large to be stored in data type\n\nInterestingly that does not seem to be a good description for the case\nPOSIX describes above.\n\n> For EILSEQ, it is\n> \n> \tIllegal byte sequence\n> \n> Neither is perfect, but the latter seems like a better fit for the kind\n> of file format errors we're describing here.\n\nI find that one actively misleading. It's not the bytes we saw, it's the\nlack of them.\n\n> Of course, it's even\n> better if we fix the callers and don't try to wedge this into errno.\n\nYes. This patch is just a stop-gap. Perhaps we should abandon it\nentirely. But I couldn't find a way to fix all the callers. If you have\na function that just returns \"-1\" when it sees a read_in_full() error,\nhow does _its_ caller tell the difference?\n\nI guess the answer is that the interface of the sub-function calling\nread_in_full() needs to change.\n\n> If you are okay with the too-long/too-short confusion in EOVERFLOW, then\n> there is EMSGSIZE:\n> \n> \tMessage too long\n\nI don't really like that one either.\n\nI actually prefer \"0\" of the all of the options discussed. At least it\nis immediately clear that it is not a syscall error.\n\n-Peff\n"},{"id":"328907","messageId":"CAGZ79kZwc8bNWJfH_36soy75EpyHAMbJGhQd4rV5f-0RMjiwHQ@mail.gmail.com","threadId":"46832","inReplyTo":"20170926000117.y3solltovyueq3zc@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-26T00:06:58Z","receivedAt":"2017-09-26T00:07:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 25, 2017 at 5:01 PM, Jeff King wrote:\n\n>\n>> If you are okay with the too-long/too-short confusion in EOVERFLOW, then\n>> there is EMSGSIZE:\n>>\n>> Message too long\n>\n> I don't really like that one either.\n>\n> I actually prefer \"0\" of the all of the options discussed. At least it\n> is immediately clear that it is not a syscall error.\n>\n> -Peff\n\nAfter reading this discussion from the sideline, maybe\n\n ENODATA No message is available on the STREAM head read\nqueue (POSIX.1)\n\nis not too bad after all. Or\n\n ESPIPE Invalid seek (POSIX.1)\n\nTechnically we do not seek, but one could imagine we'd seek there\nto read?\n"},{"id":"328908","messageId":"20170926000913.6x7qhj2vw73tldxe@sigill.intra.peff.net","threadId":"46832","inReplyTo":"CAGZ79kZwc8bNWJfH_36soy75EpyHAMbJGhQd4rV5f-0RMjiwHQ@mail.gmail.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-26T00:09:13Z","receivedAt":"2017-09-26T00:09:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 05:06:58PM -0700, Stefan Beller wrote:\n\n> >> If you are okay with the too-long/too-short confusion in EOVERFLOW, then\n> >> there is EMSGSIZE:\n> >>\n> >> Message too long\n> >\n> > I don't really like that one either.\n> >\n> > I actually prefer \"0\" of the all of the options discussed. At least it\n> > is immediately clear that it is not a syscall error.\n> >\n> > -Peff\n> \n> After reading this discussion from the sideline, maybe\n> \n> ENODATA No message is available on the STREAM head read\n> queue (POSIX.1)\n> \n> is not too bad after all. Or\n> \n> ESPIPE Invalid seek (POSIX.1)\n> \n> Technically we do not seek, but one could imagine we'd seek there\n> to read?\n\nENODATA is not too bad. On my glibc system it yields \"No data available\"\nfrom strerror(), which is at least comprehensible.\n\nWe're still left with the question of whether it is defined everywhere\n(and what to fallback to when it isn't).\n\n-Peff\n"},{"id":"328909","messageId":"20170926001354.GN27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170926000117.y3solltovyueq3zc@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-26T00:13:54Z","receivedAt":"2017-09-26T00:14:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Sep 25, 2017 at 04:55:10PM -0700, Jonathan Nieder wrote:\n\n>> All that really matters is the strerror() that the user would see.\n>\n> Agreed, though I didn't think those were necessarily standardized.\n\nThe standard allows them to vary for the sake of internationalization,\nbut they are de facto constants (probably because of application authors\nlike us abusing them ;-)).\n\n[...]\n>> Of course, it's even\n>> better if we fix the callers and don't try to wedge this into errno.\n>\n> Yes. This patch is just a stop-gap. Perhaps we should abandon it\n> entirely. But I couldn't find a way to fix all the callers. If you have\n> a function that just returns \"-1\" when it sees a read_in_full() error,\n> how does _its_ caller tell the difference?\n>\n> I guess the answer is that the interface of the sub-function calling\n> read_in_full() needs to change.\n\nYes. :/\n\n>> If you are okay with the too-long/too-short confusion in EOVERFLOW, then\n>> there is EMSGSIZE:\n>>\n>> \tMessage too long\n>\n> I don't really like that one either.\n>\n> I actually prefer \"0\" of the all of the options discussed. At least it\n> is immediately clear that it is not a syscall error.\n\nI have a basic aversion to \": Success\" in error messages. Whenever I\nsee such an error message, I stop trusting the program that produced it\nnot to be seriously buggy. Maybe I'm the only one?\n\nIf no actual errno works, we could make a custom strerror-like function\nwith our own custom errors (making them negative to avoid clashing with\nstandard errno values), but this feels like overkill.\n\nIn the same spirit of misleadingly confusing too-long and too-short,\nthere is also ERANGE (\"Result too large\"), which doesn't work here.\nThere's also EPROTO \"Protocol error\", but that's about protocols, not\nfile formats. More vague and therefor maybe more applicable is EBADMSG\n\"Bad message\". There's also ENOMSG \"No message of the desired type\".\n\nIf the goal is to support debugging, an error like EPIPE \"Broken pipe\"\nor ESPIPE \"Invalid seek\" would be likely to lead me in the right\ndirection (wrong file size), even though it is misleading about how\nthe error surfaced.\n\nWe could also avoid trying to be cute and use something generic like\nEIO \"Input/output error\".\n\nJonathan\n"},{"id":"328910","messageId":"20170926001602.GO27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170926000913.6x7qhj2vw73tldxe@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-26T00:16:02Z","receivedAt":"2017-09-26T00:16:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Mon, Sep 25, 2017 at 08:09:13PM -0400, Jeff King wrote:\n> On Mon, Sep 25, 2017 at 05:06:58PM -0700, Stefan Beller wrote:\n\n>> After reading this discussion from the sideline, maybe\n>>\n>> ENODATA No message is available on the STREAM head read\n>> queue (POSIX.1)\n>>\n>> is not too bad after all. Or\n>>\n>> ESPIPE Invalid seek (POSIX.1)\n>>\n>> Technically we do not seek, but one could imagine we'd seek there\n>> to read?\n>\n> ENODATA is not too bad. On my glibc system it yields \"No data available\"\n> from strerror(), which is at least comprehensible.\n>\n> We're still left with the question of whether it is defined everywhere\n> (and what to fallback to when it isn't).\n\nSo,\n\n#ifndef EUNDERFLOW\n#ifdef ENODATA\n#define ENODATA EUNDERFLOW\n#else\n#define ENODATA ESPIPE\n#endif\n#endif\n\n? Windows has ESPIPE, and I'm not sure what other non-POSIX platform we\nneed to worry about.\n\nThanks,\nJonathan\n"},{"id":"328911","messageId":"20170926001724.cxuwsdadjbqqmnig@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170926001354.GN27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-26T00:17:24Z","receivedAt":"2017-09-26T00:17:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 05:13:54PM -0700, Jonathan Nieder wrote:\n\n> > I actually prefer \"0\" of the all of the options discussed. At least it\n> > is immediately clear that it is not a syscall error.\n> \n> I have a basic aversion to \": Success\" in error messages. Whenever I\n> see such an error message, I stop trusting the program that produced it\n> not to be seriously buggy. Maybe I'm the only one?\n\nThat's a fair criticism, I think.\n\n> If no actual errno works, we could make a custom strerror-like function\n> with our own custom errors (making them negative to avoid clashing with\n> standard errno values), but this feels like overkill.\n\nYes, I also thought about that. It feels pretty invasive (I guess we'd\nwrap strerror() with our own custom function so that callers don't have\nto remember which one to use).\n\n> In the same spirit of misleadingly confusing too-long and too-short,\n> there is also ERANGE (\"Result too large\"), which doesn't work here.\n> There's also EPROTO \"Protocol error\", but that's about protocols, not\n> file formats. More vague and therefor maybe more applicable is EBADMSG\n> \"Bad message\". There's also ENOMSG \"No message of the desired type\".\n> \n> If the goal is to support debugging, an error like EPIPE \"Broken pipe\"\n> or ESPIPE \"Invalid seek\" would be likely to lead me in the right\n> direction (wrong file size), even though it is misleading about how\n> the error surfaced.\n> \n> We could also avoid trying to be cute and use something generic like\n> EIO \"Input/output error\".\n\nI definitely would prefer to avoid EIO, or anything that an actual\nread() might return.\n\nWhat do you think of ENODATA? The glibc text for it is pretty\nappropriate. If it's not available everywhere, we'd have to fallback to\nsomething else (EIO? 0?). I don't know how esoteric it is. The likely\ncandidate to be lacking it is Windows.\n\n-Peff\n"},{"id":"328912","messageId":"20170926001732.GP27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170926001602.GO27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-26T00:17:32Z","receivedAt":"2017-09-26T00:17:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> On Mon, Sep 25, 2017 at 08:09:13PM -0400, Jeff King wrote:\n\n>> ENODATA is not too bad. On my glibc system it yields \"No data available\"\n>> from strerror(), which is at least comprehensible.\n>>\n>> We're still left with the question of whether it is defined everywhere\n>> (and what to fallback to when it isn't).\n>\n> So,\n>\n> #ifndef EUNDERFLOW\n> #ifdef ENODATA\n> #define ENODATA EUNDERFLOW\n> #else\n> #define ENODATA ESPIPE\n> #endif\n> #endif\n>\n> ?\n\nUh, pretend I said\n\n#ifndef EUNDERFLOW\n# ifdef ENODATA\n# define EUNDERFLOW ENODATA\n# else\n# define EUNDERFLOW ESPIPE\n# endif\n#endif\n\nSorry for the nonsense.\nJonathan\n"},{"id":"328913","messageId":"20170926001956.hgtdeklogvkirdxa@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170926001732.GP27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-26T00:19:57Z","receivedAt":"2017-09-26T00:20:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 05:17:32PM -0700, Jonathan Nieder wrote:\n\n> Jonathan Nieder wrote:\n> > On Mon, Sep 25, 2017 at 08:09:13PM -0400, Jeff King wrote:\n> \n> >> ENODATA is not too bad. On my glibc system it yields \"No data available\"\n> >> from strerror(), which is at least comprehensible.\n> >>\n> >> We're still left with the question of whether it is defined everywhere\n> >> (and what to fallback to when it isn't).\n> >\n> > So,\n> >\n> > #ifndef EUNDERFLOW\n> > #ifdef ENODATA\n> > #define ENODATA EUNDERFLOW\n> > #else\n> > #define ENODATA ESPIPE\n> > #endif\n> > #endif\n> >\n> > ?\n> \n> Uh, pretend I said\n> \n> #ifndef EUNDERFLOW\n> # ifdef ENODATA\n> # define EUNDERFLOW ENODATA\n> # else\n> # define EUNDERFLOW ESPIPE\n> # endif\n> #endif\n\nRight, I think our mails just crossed but I'm leaning in this direction.\nI'd prefer to call it SHORT_READ_ERRNO or something, though. Your\n\"#ifndef EUNDERFLOW\" had me thinking that this was something that a real\nplatform might have, but AFAICT you just made it up.\n\n-Peff\n"},{"id":"328914","messageId":"20170926002020.GQ27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170926001724.cxuwsdadjbqqmnig@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-26T00:20:20Z","receivedAt":"2017-09-26T00:20:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I definitely would prefer to avoid EIO, or anything that an actual\n> read() might return.\n>\n> What do you think of ENODATA? The glibc text for it is pretty\n> appropriate. If it's not available everywhere, we'd have to fallback to\n> something else (EIO? 0?). I don't know how esoteric it is. The likely\n> candidate to be lacking it is Windows.\n\nENODATA with a fallback to ESPIPE sounds fine to me.\n\nread() would never produce ESPIPE because it doesn't seek.\n\nSo that would be:\n\n/* errno value to use for early EOF */\n#ifndef ENODATA\n#define ENODATA ESPIPE\n#endif\n\nThanks,\nJonathan\n"},{"id":"328915","messageId":"20170926002246.GR27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170926001956.hgtdeklogvkirdxa@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-26T00:22:46Z","receivedAt":"2017-09-26T00:22:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Sep 25, 2017 at 05:17:32PM -0700, Jonathan Nieder wrote:\n\n>> #ifndef EUNDERFLOW\n>> # ifdef ENODATA\n>> # define EUNDERFLOW ENODATA\n>> # else\n>> # define EUNDERFLOW ESPIPE\n>> # endif\n>> #endif\n>\n> Right, I think our mails just crossed but I'm leaning in this direction.\n> I'd prefer to call it SHORT_READ_ERRNO or something, though. Your\n> \"#ifndef EUNDERFLOW\" had me thinking that this was something that a real\n> platform might have, but AFAICT you just made it up.\n\nAgreed. Two of the risks of replying too quickly. :)\n\nJonathan\n"},{"id":"328916","messageId":"20170926002510.xmusj6fkvb25fm5w@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170926002020.GQ27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-26T00:25:10Z","receivedAt":"2017-09-26T00:25:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 05:20:20PM -0700, Jonathan Nieder wrote:\n\n> > What do you think of ENODATA? The glibc text for it is pretty\n> > appropriate. If it's not available everywhere, we'd have to fallback to\n> > something else (EIO? 0?). I don't know how esoteric it is. The likely\n> > candidate to be lacking it is Windows.\n> \n> ENODATA with a fallback to ESPIPE sounds fine to me.\n> \n> read() would never produce ESPIPE because it doesn't seek.\n> \n> So that would be:\n> \n> /* errno value to use for early EOF */\n> #ifndef ENODATA\n> #define ENODATA ESPIPE\n> #endif\n\nThanks, I'll re-roll with that (and thank you Stefan for mentioning\nENODATA again; I came across it early in my search but discounted it as\nnot quite right semantically. I should have been looking at the\nstrerror() text, which is what really matters).\n\nWhat do you think of patch 7 in light of this? If the short-read case\ngives us a sane errno, should we even bother trying to consistently\nhandle its error separately?\n\n-Peff\n"},{"id":"328917","messageId":"20170926002805.GS27425@aiede.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170926002510.xmusj6fkvb25fm5w@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-26T00:28:05Z","receivedAt":"2017-09-26T00:28:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> What do you think of patch 7 in light of this? If the short-read case\n> gives us a sane errno, should we even bother trying to consistently\n> handle its error separately?\n\nI like the read_exactly_or_die variant because it makes callers more\nconcise, but on the other hand a caller handling the error can write a\nmore meaningful error message with the right amount of context.\n\nSo I think you're right that it's better to drop patch 7.\n\nThanks,\nJonathan\n"},{"id":"328924","messageId":"xmqqvak6ayk0.fsf@gitster.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170926001956.hgtdeklogvkirdxa@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-26T04:11:59Z","receivedAt":"2017-09-26T04:12:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> #ifndef EUNDERFLOW\n>> # ifdef ENODATA\n>> # define EUNDERFLOW ENODATA\n>> # else\n>> # define EUNDERFLOW ESPIPE\n>> # endif\n>> #endif\n>\n> Right, I think our mails just crossed but I'm leaning in this direction.\n\nHmph, I may be slow (or may be skimming the exchanges too fast), but\nwhat exactly is wrong with \"0\"? As long as we do not have to tell\ntwo or more \"not exactly an error from syscall in errno\" cases, I\nwould think \"0\" is the best value to use.\n\nIf the syserror message _is_ the issue, then we'd need to either\npick an existing errno that is available everywhere (with possibly\nsuboptimal message), or pick something and prepare a fallback for\nplatforms that lack the errno, so picking \"0\" as the value and use\nwhatever logic we would have used for the \"fallback\" would not sound\ntoo bad. I.e.\n\n\tif (read_in_full(..., size) != size)\n\t\tif (errno)\n\t\t\tdie_errno(\"oops\");\n\t\telse\n\t\t\tdie(\"short read\");\n\nIf the callsite were too numerous,\n\n#define die_errno_or(msg1, msg2) if (errno) die_errno(msg1); else die(msg2)\n\nperhaps?\n\n"},{"id":"328997","messageId":"20170927040721.6av4o7cjrrkzizzo@sigill.intra.peff.net","threadId":"46832","inReplyTo":"xmqqvak6ayk0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T04:07:22Z","receivedAt":"2017-09-27T04:07:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 26, 2017 at 01:11:59PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> #ifndef EUNDERFLOW\n> >> # ifdef ENODATA\n> >> # define EUNDERFLOW ENODATA\n> >> # else\n> >> # define EUNDERFLOW ESPIPE\n> >> # endif\n> >> #endif\n> >\n> > Right, I think our mails just crossed but I'm leaning in this direction.\n> \n> Hmph, I may be slow (or may be skimming the exchanges too fast), but\n> what exactly is wrong with \"0\"? As long as we do not have to tell\n> two or more \"not exactly an error from syscall in errno\" cases, I\n> would think \"0\" is the best value to use.\n\nThe main reason to avoid \"0\" is just that the \"read error: Success\"\nmessage is a bit funny.\n\n> If the syserror message _is_ the issue, then we'd need to either\n> pick an existing errno that is available everywhere (with possibly\n> suboptimal message), or pick something and prepare a fallback for\n> platforms that lack the errno, so picking \"0\" as the value and use\n> whatever logic we would have used for the \"fallback\" would not sound\n> too bad. I.e.\n> \n> \tif (read_in_full(..., size) != size)\n> \t\tif (errno)\n> \t\t\tdie_errno(\"oops\");\n> \t\telse\n> \t\t\tdie(\"short read\");\n> \n> If the callsite were too numerous,\n\nYou actually don't need errno for that. You can write:\n\n ret = read_in_full(..., size);\n if (ret < 0)\n\tdie_errno(\"oops\");\n else if (ret != size)\n\tdie(\"short read\");\n\nSo I think using errno as a sentinel value to tell between the two cases\ndoesn't have much value.\n\n> #define die_errno_or(msg1, msg2) if (errno) die_errno(msg1); else die(msg2)\n> \n> perhaps?\n\nIf you just care about dying, you can wrap the whole read_in_full() in a\nhelper (which is what my original patch 7 did). But I found there were\ncases that didn't want to die. Of course _most_ of those don't bother\nlooking at errno in the first place. I think the only one that does is\nindex_core(), which calls error_errno().\n\nSo with two helpers (one for die and one for error), I think we could\ncover the existing cases. Or we could convert the single error() case to\njust handle the logic inline.\n\nOne of the reasons I dislike the helper I showed earlier is that it\ninvolves assembling a lego sentence. But the alternative is requiring\nthe caller to write both sentences from scratch (and writing a good\nsentence for the short read really is long: you expected X bytes but got\nY).\n\nI guess just tweaking the errno value does not really give a good \"short\nread\" error either. You get \"unable to read foo: No data available\",\nwithout the X/Y information.\n\nSometimes a custom \"short read\" message can be better, if it's \"could\nnot read enough bytes for packfile header\" or similar. You don't need to\nknow the exact number then.\n\nSo I dunno. Maybe I have just talked myself into actually just fixing\nall of the read_in_full() call sites manually.\n\n-Peff\n"},{"id":"328998","messageId":"xmqqa81g7ol9.fsf@gitster.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170927040721.6av4o7cjrrkzizzo@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] read_in_full: reset errno before reading","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T04:27:46Z","receivedAt":"2017-09-27T04:27:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You actually don't need errno for that. You can write:\n>\n> ret = read_in_full(..., size);\n> if (ret < 0)\n> \tdie_errno(\"oops\");\n> else if (ret != size)\n> \tdie(\"short read\");\n>\n> So I think using errno as a sentinel value to tell between the two cases\n> doesn't have much value.\n> ...\n> One of the reasons I dislike the helper I showed earlier is that it\n> involves assembling a lego sentence. But the alternative is requiring\n> the caller to write both sentences from scratch (and writing a good\n> sentence for the short read really is long: you expected X bytes but got\n> Y).\n\nAll sensible considerations. I probably should have read the entire\ndiscussion twice before starting to type X-<. Sorry about the\nnoise.\n\n> Sometimes a custom \"short read\" message can be better, if it's \"could\n> not read enough bytes for packfile header\" or similar. You don't need to\n> know the exact number then.\n\nThat's true, too. I dunno, either.\n"},{"id":"329004","messageId":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170925202646.agsnpmar3dzocdcr@sigill.intra.peff.net","subject":"[PATCH v2 0/7] read/write_in_full leftovers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T05:54:24Z","receivedAt":"2017-09-27T05:54:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 25, 2017 at 04:26:47PM -0400, Jeff King wrote:\n\n> This series addresses the bits leftover from the discussion two weeks\n> ago in:\n> \n> https://public-inbox.org/git/20170913170807.cyx7rrpoyhaauvol@sigill.intra.peff.net/\n> \n> and its subthread. I don't think any of these is a real problem in\n> practice, so this can be considered as a cleanup. I'm on the fence over\n> whether the final one is a good idea. See the commit message for\n> details.\n> \n> [1/7]: files-backend: prefer \"0\" for write_in_full() error check\n> [2/7]: notes-merge: drop dead zero-write code\n> [3/7]: read_in_full: reset errno before reading\n> [4/7]: get-tar-commit-id: prefer \"!=\" for read_in_full() error check\n> [5/7]: worktree: use xsize_t to access file size\n> [6/7]: worktree: check the result of read_in_full()\n> [7/7]: add xread_in_full() helper\n\nHere's a followup based on the discussion on v1. There are actually a\nfew changes here.\n\nI dropped the \"read_in_full() should set errno on short reads\" idea (3/7\nin the earlier series). It really is the caller's fault for looking at\nerrno when they know there hasn't been an error in the first place. We\nshould just bite the bullet and have the callers do the right thing.\n\nI also dropped the \"xread_in_full\" helper (7/7 earlier). The lego\nsentences it created just weren't worth the hassle. Instead, I've fixed\nall of the relevant callers to provide good error messages for both\ncases. It's a few more lines of code, and it's probably rare for users\nto see these in the first place. But it doesn't hurt too much to be\nthorough, and I think it's good to model correct error handling. This is\nin patches 4 and 5 below.\n\nThere are a handful of other changes:\n\n [1/7]: files-backend: prefer \"0\" for write_in_full() error check\n [2/7]: notes-merge: drop dead zero-write code\n\n These two are the same as before.\n\n [3/7]: prefer \"!=\" when checking read_in_full() result\n\n Along with the get-tar-commit-id case, I found two other cases which\n aren't bugs, but which should be updated for style reasons.\n\n [4/7]: avoid looking at errno for short read_in_full() returns\n [5/7]: distinguish error versus short_read from read_in_full()\n\n These two are new, and add in the messages. The ones in patch 4 are\n actively wrong (they show a bogus errno). The ones in 5 are just\n less descriptive than they could be. So if we find the extra lines\n of code too much, we could drop 5 (or even convert the ones in 4 to\n just stop looking at errno).\n\n [6/7]: worktree: use xsize_t to access file size\n\n Same as before.\n\n [7/7]: worktree: check the result of read_in_full()\n\n Similar, but now handles errors and short_reads separately. It also\n avoids leaking descriptors and memory in the error path (noticed by\n Coverity).\n\n builtin/get-tar-commit-id.c | 6 ++++--\n builtin/worktree.c | 24 +++++++++++++++++++++---\n bulk-checkin.c | 5 ++++-\n csum-file.c | 2 +-\n notes-merge.c | 2 --\n pack-write.c | 7 ++++++-\n packfile.c | 11 +++++++++--\n pkt-line.c | 2 +-\n refs/files-backend.c | 2 +-\n sha1_file.c | 11 ++++++++---\n 10 files changed, 55 insertions(+), 17 deletions(-)\n\n-Peff\n"},{"id":"329005","messageId":"20170927060000.z2bs7lojzh6wprhr@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"[PATCH v2 1/7] files-backend: prefer \"0\" for write_in_full() error check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:00:01Z","receivedAt":"2017-09-27T06:00:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 06f46f237a (avoid \"write_in_full(fd, buf, len) !=\nlen\" pattern, 2017-09-13) converted this callsite from:\n\n write_in_full(...) != 1\n\nto\n\n write_in_full(...) < 0\n\nBut during the conflict resolution in c50424a6f0 (Merge\nbranch 'jk/write-in-full-fix', 2017-09-25), this morphed\ninto\n\n write_in_full(...) < 1\n\nThis behaves as we want, but we prefer to avoid modeling the\n\"less than length\" error-check which can be subtly buggy, as\nshown in efacf609c8 (config: avoid \"write_in_full(fd, buf,\nlen) < len\" pattern, 2017-09-13).\n\nSigned-off-by: Jeff King \n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex dac33628b3..e35c64c001 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3007,7 +3007,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t} else if (update &&\n \t\t\t (write_in_full(get_lock_file_fd(&lock->lk),\n \t\t\t\toid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||\n-\t\t\t write_str_in_full(get_lock_file_fd(&lock->lk), \"\\n\") < 1 ||\n+\t\t\t write_str_in_full(get_lock_file_fd(&lock->lk), \"\\n\") < 0 ||\n \t\t\t close_ref_gently(lock) < 0)) {\n \t\t\tstatus |= error(\"couldn't write %s\",\n \t\t\t\t\tget_lock_file_path(&lock->lk));\n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329006","messageId":"20170927060007.hcoesuhxqtwvdjj3@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"[PATCH v2 2/7] notes-merge: drop dead zero-write code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:00:08Z","receivedAt":"2017-09-27T06:00:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We call write_in_full() with a size that we know is greater\nthan zero. The return value can never be zero, then, since\nwrite_in_full() converts such a failed write() into ENOSPC\nand returns -1. We can just drop this branch of the error\nhandling entirely.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Jeff King \n---\n notes-merge.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 597d43f65c..4352c34a6e 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -308,8 +308,6 @@ static void write_buf_to_worktree(const struct object_id *obj,\n \t\t\tif (errno == EPIPE)\n \t\t\t\tbreak;\n \t\t\tdie_errno(\"notes-merge\");\n-\t\t} else if (!ret) {\n-\t\t\tdie(\"notes-merge: disk full?\");\n \t\t}\n \t\tsize -= ret;\n \t\tbuf += ret;\n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329007","messageId":"20170927060028.ehddn5zwf6v6jsvs@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"[PATCH v2 3/7] prefer \"!=\" when checking read_in_full() result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:00:28Z","receivedAt":"2017-09-27T06:00:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Comparing the result of read_in_full() using less-than is\npotentially dangerous, as a negative return value may be\nconverted to an unsigned type and be considered a success.\nThis is discussed further in 561598cfcf (read_pack_header:\nhandle signed/unsigned comparison in read result,\n2017-09-13).\n\nEach of these instances is actually fine in practice:\n\n - in get-tar-commit-id, the HEADERSIZE macro expands to a\n signed integer. If it were switched to an unsigned type\n (e.g., a size_t), then it would be a bug.\n\n - the other two callers check for a short read only after\n handling a negative return separately. This is a fine\n practice, but we'd prefer to model \"!=\" as a general\n rule.\n\nSo all of these cases can be considered cleanups and not\nactual bugfixes.\n\nSigned-off-by: Jeff King \n---\n builtin/get-tar-commit-id.c | 2 +-\n csum-file.c | 2 +-\n pkt-line.c | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 6d9a79f9b3..cd3e656828 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -26,7 +26,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \n \tn = read_in_full(0, buffer, HEADERSIZE);\n-\tif (n < HEADERSIZE)\n+\tif (n != HEADERSIZE)\n \t\tdie(\"git get-tar-commit-id: read error\");\n \tif (header->typeflag[0] != 'g')\n \t\treturn 1;\ndiff --git a/csum-file.c b/csum-file.c\nindex a172199e44..2adae04073 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -19,7 +19,7 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)\n \n \t\tif (ret < 0)\n \t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n-\t\tif (ret < count)\n+\t\tif (ret != count)\n \t\t\tdie(\"%s: sha1 file truncated\", f->name);\n \t\tif (memcmp(buf, check_buffer, count))\n \t\t\tdie(\"sha1 file '%s' validation error\", f->name);\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 647bbd3bce..93ea311443 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -258,7 +258,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \t}\n \n \t/* And complain if we didn't get enough bytes to satisfy the read. */\n-\tif (ret < size) {\n+\tif (ret != size) {\n \t\tif (options & PACKET_READ_GENTLE_ON_EOF)\n \t\t\treturn -1;\n \n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329008","messageId":"20170927060107.duocwl3mf62zvgjp@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"[PATCH v2 4/7] avoid looking at errno for short read_in_full() returns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:01:07Z","receivedAt":"2017-09-27T06:01:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a caller tries to read a particular set of bytes via\nread_in_full(), there are three possible outcomes:\n\n 1. An error, in which case -1 is returned and errno is\n set.\n\n 2. A short read, in which fewer bytes are returned and\n errno is unspecified (we never saw a read error, so we\n may have some random value from whatever syscall failed\n last).\n\n 3. The full read completed successfully.\n\nMany callers handle cases 1 and 2 together by just checking\nthe result against the requested size. If their combined\nerror path looks at errno (e.g., by calling die_errno), they\nmay report a nonsense value.\n\nLet's fix these sites by having them distinguish between the\ntwo error cases. That avoids the random errno confusion, and\nlets us give more detailed error messages.\n\nSigned-off-by: Jeff King \n---\n pack-write.c | 7 ++++++-\n sha1_file.c | 11 ++++++++---\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex a333ec6754..fea6284192 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -213,14 +213,19 @@ void fixup_pack_header_footer(int pack_fd,\n \tgit_SHA_CTX old_sha1_ctx, new_sha1_ctx;\n \tstruct pack_header hdr;\n \tchar *buf;\n+\tssize_t read_result;\n \n \tgit_SHA1_Init(&old_sha1_ctx);\n \tgit_SHA1_Init(&new_sha1_ctx);\n \n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n \t\tdie_errno(\"Failed seeking to start of '%s'\", pack_name);\n-\tif (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n+\tread_result = read_in_full(pack_fd, &hdr, sizeof(hdr));\n+\tif (read_result < 0)\n \t\tdie_errno(\"Unable to reread header of '%s'\", pack_name);\n+\telse if (read_result != sizeof(hdr))\n+\t\tdie_errno(\"Unexpected short read for header of '%s'\",\n+\t\t\t pack_name);\n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n \t\tdie_errno(\"Failed seeking to start of '%s'\", pack_name);\n \tgit_SHA1_Update(&old_sha1_ctx, &hdr, sizeof(hdr));\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5a2014811f..09ad64ce55 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1748,10 +1748,15 @@ static int index_core(unsigned char *sha1, int fd, size_t size,\n \t\tret = index_mem(sha1, \"\", size, type, path, flags);\n \t} else if (size <= SMALL_FILE_SIZE) {\n \t\tchar *buf = xmalloc(size);\n-\t\tif (size == read_in_full(fd, buf, size))\n-\t\t\tret = index_mem(sha1, buf, size, type, path, flags);\n+\t\tssize_t read_result = read_in_full(fd, buf, size);\n+\t\tif (read_result < 0)\n+\t\t\tret = error_errno(\"read error while indexing %s\",\n+\t\t\t\t\t path ? path : \"\");\n+\t\telse if (read_result != size)\n+\t\t\tret = error(\"short read while indexing %s\",\n+\t\t\t\t path ? path : \"\");\n \t\telse\n-\t\t\tret = error_errno(\"short read\");\n+\t\t\tret = index_mem(sha1, buf, size, type, path, flags);\n \t\tfree(buf);\n \t} else {\n \t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329009","messageId":"20170927060210.euuaefuakciusudl@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"[PATCH v2 5/7] distinguish error versus short read from read_in_full()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:02:11Z","receivedAt":"2017-09-27T06:02:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many callers of read_in_full() expect to see the exact\nnumber of bytes requested, but their error handling lumps\ntogether true read errors and short reads due to unexpected\nEOF.\n\nWe can give more specific error messages by separating these\ncases (showing errno when appropriate, and otherwise\ndescribing the short read).\n\nSigned-off-by: Jeff King \n---\n builtin/get-tar-commit-id.c | 4 +++-\n bulk-checkin.c | 5 ++++-\n packfile.c | 11 +++++++++--\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex cd3e656828..2706fcfaf2 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -26,8 +26,10 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \n \tn = read_in_full(0, buffer, HEADERSIZE);\n+\tif (n < 0)\n+\t\tdie_errno(\"git get-tar-commit-id: read error\");\n \tif (n != HEADERSIZE)\n-\t\tdie(\"git get-tar-commit-id: read error\");\n+\t\tdie_errno(\"git get-tar-commit-id: EOF before reading tar header\");\n \tif (header->typeflag[0] != 'g')\n \t\treturn 1;\n \tif (!skip_prefix(content, \"52 comment=\", &comment))\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 9a1f6c49ab..3310fd210a 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -115,7 +115,10 @@ static int stream_to_pack(struct bulk_checkin_state *state,\n \n \t\tif (size && !s.avail_in) {\n \t\t\tssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);\n-\t\t\tif (read_in_full(fd, ibuf, rsize) != rsize)\n+\t\t\tssize_t read_result = read_in_full(fd, ibuf, rsize);\n+\t\t\tif (read_result < 0)\n+\t\t\t\tdie_errno(\"failed to read from '%s'\", path);\n+\t\t\tif (read_result != rsize)\n \t\t\t\tdie(\"failed to read %d bytes from '%s'\",\n \t\t\t\t (int)rsize, path);\n \t\t\toffset += rsize;\ndiff --git a/packfile.c b/packfile.c\nindex f69a5c8d60..eab7542487 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -442,6 +442,7 @@ static int open_packed_git_1(struct packed_git *p)\n \tunsigned char sha1[20];\n \tunsigned char *idx_sha1;\n \tlong fd_flag;\n+\tssize_t read_result;\n \n \tif (!p->index_data && open_pack_index(p))\n \t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n@@ -483,7 +484,10 @@ static int open_packed_git_1(struct packed_git *p)\n \t\treturn error(\"cannot set FD_CLOEXEC\");\n \n \t/* Verify we recognize this pack file format. */\n-\tif (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n+\tread_result = read_in_full(p->pack_fd, &hdr, sizeof(hdr));\n+\tif (read_result < 0)\n+\t\treturn error_errno(\"error reading from %s\", p->pack_name);\n+\tif (read_result != sizeof(hdr))\n \t\treturn error(\"file %s is far too short to be a packfile\", p->pack_name);\n \tif (hdr.hdr_signature != htonl(PACK_SIGNATURE))\n \t\treturn error(\"file %s is not a GIT packfile\", p->pack_name);\n@@ -500,7 +504,10 @@ static int open_packed_git_1(struct packed_git *p)\n \t\t\t p->num_objects);\n \tif (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)\n \t\treturn error(\"end of packfile %s is unavailable\", p->pack_name);\n-\tif (read_in_full(p->pack_fd, sha1, sizeof(sha1)) != sizeof(sha1))\n+\tread_result = read_in_full(p->pack_fd, sha1, sizeof(sha1));\n+\tif (read_result < 0)\n+\t\treturn error_errno(\"error reading from %s\", p->pack_name);\n+\tif (read_result != sizeof(sha1))\n \t\treturn error(\"packfile %s signature is unavailable\", p->pack_name);\n \tidx_sha1 = ((unsigned char *)p->index_data) + p->index_size - 40;\n \tif (hashcmp(sha1, idx_sha1))\n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329010","messageId":"20170927060221.pv27abu7wmogm5dx@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"[PATCH v2 6/7] worktree: use xsize_t to access file size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:02:21Z","receivedAt":"2017-09-27T06:02:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To read the \"gitdir\" file into memory, we stat the file and\nallocate a buffer. But we store the size in an \"int\", which\nmay be truncated. We should use a size_t and xsize_t(),\nwhich will detect truncation.\n\nAn overflow is unlikely for a \"gitdir\" file, but it's a good\npractice to model.\n\nSigned-off-by: Jeff King \n---\n builtin/worktree.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex de26849f55..2f4a4ef9cd 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -38,7 +38,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n {\n \tstruct stat st;\n \tchar *path;\n-\tint fd, len;\n+\tint fd;\n+\tsize_t len;\n \n \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n \t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: not a valid directory\"), id);\n@@ -56,7 +57,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \t\t\t id, strerror(errno));\n \t\treturn 1;\n \t}\n-\tlen = st.st_size;\n+\tlen = xsize_t(st.st_size);\n \tpath = xmallocz(len);\n \tread_in_full(fd, path, len);\n \tclose(fd);\n-- \n2.14.2.988.g01c8b37dde\n\n"},{"id":"329011","messageId":"20170927060226.a7odofrkyueazp34@sigill.intra.peff.net","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"[PATCH v2 7/7] worktree: check the result of read_in_full()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T06:02:27Z","receivedAt":"2017-09-27T06:02:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We try to read \"len\" bytes into a buffer and just assume\nthat it happened correctly. In practice this should usually\nbe the case, since we just stat'd the file to get the\nlength. But we could be fooled by transient errors or by\nother processes racily truncating the file.\n\nLet's be more careful. There's a slim chance this could\ncatch a real error, but it also prevents people and tools\nfrom getting worried while reading the code.\n\nSigned-off-by: Jeff King \n---\n builtin/worktree.c | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 2f4a4ef9cd..7b9307aa58 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -40,6 +40,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \tchar *path;\n \tint fd;\n \tsize_t len;\n+\tssize_t read_result;\n \n \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n \t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: not a valid directory\"), id);\n@@ -59,8 +60,24 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \t}\n \tlen = xsize_t(st.st_size);\n \tpath = xmallocz(len);\n-\tread_in_full(fd, path, len);\n+\n+\tread_result = read_in_full(fd, path, len);\n+\tif (read_result < 0) {\n+\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: unable to read gitdir file (%s)\"),\n+\t\t\t id, strerror(errno));\n+\t\tclose(fd);\n+\t\tfree(path);\n+\t\treturn 1;\n+\t}\n \tclose(fd);\n+\n+\tif (read_result != len) {\n+\t\tstrbuf_addf(reason,\n+\t\t\t _(\"Removing worktrees/%s: short read (expected %\"PRIuMAX\" bytes, read %\"PRIuMAX\")\"),\n+\t\t\t id, (uintmax_t)len, (uintmax_t)read_result);\n+\t\tfree(path);\n+\t\treturn 1;\n+\t}\n \twhile (len && (path[len - 1] == '\\n' || path[len - 1] == '\\r'))\n \t\tlen--;\n \tif (!len) {\n-- \n2.14.2.988.g01c8b37dde\n"},{"id":"329023","messageId":"xmqqy3p04oi2.fsf@gitster.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170927055424.22ati3g24xaqtqrk@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/7] read/write_in_full leftovers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T06:57:57Z","receivedAt":"2017-09-27T06:58:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I dropped the \"read_in_full() should set errno on short reads\" idea (3/7\n> in the earlier series). It really is the caller's fault for looking at\n> errno when they know there hasn't been an error in the first place. We\n> should just bite the bullet and have the callers do the right thing.\n>\n> I also dropped the \"xread_in_full\" helper (7/7 earlier). The lego\n> sentences it created just weren't worth the hassle. Instead, I've fixed\n> all of the relevant callers to provide good error messages for both\n> cases. It's a few more lines of code, and it's probably rare for users\n> to see these in the first place. But it doesn't hurt too much to be\n> thorough, and I think it's good to model correct error handling. This is\n> in patches 4 and 5 below.\n\nThanks for being thorough. My comment on 3/7 might be taken as\ncontradicting with how 5/7 ties the loose ends up, but I do not care\ntoo deeply either way.\n\nWill queue.\n"},{"id":"329024","messageId":"xmqqr2us4ofw.fsf@gitster.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170927060028.ehddn5zwf6v6jsvs@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/7] prefer \"!=\" when checking read_in_full() result","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T06:59:15Z","receivedAt":"2017-09-27T06:59:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/csum-file.c b/csum-file.c\n> index a172199e44..2adae04073 100644\n> --- a/csum-file.c\n> +++ b/csum-file.c\n> @@ -19,7 +19,7 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)\n> \n> \t\tif (ret < 0)\n> \t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n> -\t\tif (ret < count)\n> +\t\tif (ret != count)\n> \t\t\tdie(\"%s: sha1 file truncated\", f->name);\n\nI personally find that this \"ret < count\" that comes after checking\nif ret is negative expresses what it is checking in a more natural\nway than \"ret must be exactly count\".\n\nIt is not a big deal, as either one needs to be read with an\nunderstanding that read_in_full() would read at most \"count\" bytes\nto see if the right condition is being checked to declare\n\"truncated\" anyway. But I somehow find\n\n\tret = read up to count\n\tif (ret < 0)\n\t\tread failed\n\tif (ret < count)\n\t\twe failed to read as much as expected\n\na bit more natural.\n\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 647bbd3bce..93ea311443 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -258,7 +258,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n> \t}\n> \n> \t/* And complain if we didn't get enough bytes to satisfy the read. */\n> -\tif (ret < size) {\n> +\tif (ret != size) {\n> \t\tif (options & PACKET_READ_GENTLE_ON_EOF)\n> \t\t\treturn -1;\n\nLikewise, even though it is harder to see that this follows another\nexplicit check for \"ret < 0\".\n\nThanks.\n"},{"id":"329025","messageId":"xmqqk20k4ofn.fsf@gitster.mtv.corp.google.com","threadId":"46832","inReplyTo":"20170927060107.duocwl3mf62zvgjp@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/7] avoid looking at errno for short read_in_full() returns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T06:59:24Z","receivedAt":"2017-09-27T06:59:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When a caller tries to read a particular set of bytes via\n> read_in_full(), there are three possible outcomes:\n>\n> 1. An error, in which case -1 is returned and errno is\n> set.\n>\n> 2. A short read, in which fewer bytes are returned and\n> errno is unspecified (we never saw a read error, so we\n> may have some random value from whatever syscall failed\n> last).\n>\n> 3. The full read completed successfully.\n>\n> Many callers handle cases 1 and 2 together by just checking\n> the result against the requested size. If their combined\n> error path looks at errno (e.g., by calling die_errno), they\n> may report a nonsense value.\n>\n> Let's fix these sites by having them distinguish between the\n> two error cases. That avoids the random errno confusion, and\n> lets us give more detailed error messages.\n\nThe resulting code might be more verbose but I personally think both\nof them give a lot more clear error indication.\n"},{"id":"329028","messageId":"20170927070959.snjdfrunaybo2buc@sigill.intra.peff.net","threadId":"46832","inReplyTo":"xmqqr2us4ofw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/7] prefer \"!=\" when checking read_in_full() result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T07:09:59Z","receivedAt":"2017-09-27T07:10:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 03:59:15PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > diff --git a/csum-file.c b/csum-file.c\n> > index a172199e44..2adae04073 100644\n> > --- a/csum-file.c\n> > +++ b/csum-file.c\n> > @@ -19,7 +19,7 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)\n> > \n> > \t\tif (ret < 0)\n> > \t\t\tdie_errno(\"%s: sha1 file read error\", f->name);\n> > -\t\tif (ret < count)\n> > +\t\tif (ret != count)\n> > \t\t\tdie(\"%s: sha1 file truncated\", f->name);\n> \n> I personally find that this \"ret < count\" that comes after checking\n> if ret is negative expresses what it is checking in a more natural\n> way than \"ret must be exactly count\".\n> \n> It is not a big deal, as either one needs to be read with an\n> understanding that read_in_full() would read at most \"count\" bytes\n> to see if the right condition is being checked to declare\n> \"truncated\" anyway. But I somehow find\n> \n> \tret = read up to count\n> \tif (ret < 0)\n> \t\tread failed\n> \tif (ret < count)\n> \t\twe failed to read as much as expected\n> \n> a bit more natural.\n\nI actually do, too, and I wouldn't terribly mind to drop these. Mostly I\ndidn't want people blindly copying only the second half. The fact that\nthe second condition is OK only because the first condition is present\nis somewhat subtle.\n\nI also wondered if this would shut up -Wsign-compare. But it doesn't\nseem to complain about the originals, which kind of surprises me.\n\n-Peff\n"}]}