Re: [PATCH v2 4/7] avoid looking at errno for short read_in_full() returns

[Junio C Hamano]

Jeff King  writes:

> When a caller tries to read a particular set of bytes via
> read_in_full(), there are three possible outcomes:
>
>   1. An error, in which case -1 is returned and errno is
>  set.
>
>   2. A short read, in which fewer bytes are returned and
>  errno is unspecified (we never saw a read error, so we
>  may have some random value from whatever syscall failed
>  last).
>
>   3. The full read completed successfully.
>
> Many callers handle cases 1 and 2 together by just checking
> the result against the requested size. If their combined
> error path looks at errno (e.g., by calling die_errno), they
> may report a nonsense value.
>
> Let's fix these sites by having them distinguish between the
> two error cases. That avoids the random errno confusion, and
> lets us give more detailed error messages.

The resulting code might be more verbose but I personally think both
of them give a lot more clear error indication.

[PATCH v2 4/7] avoid looking at errno for short read_in_full() returns

[Jeff King]

When a caller tries to read a particular set of bytes via
read_in_full(), there are three possible outcomes:

  1. An error, in which case -1 is returned and errno is
 set.

  2. A short read, in which fewer bytes are returned and
 errno is unspecified (we never saw a read error, so we
 may have some random value from whatever syscall failed
 last).

  3. The full read completed successfully.

Many callers handle cases 1 and 2 together by just checking
the result against the requested size. If their combined
error path looks at errno (e.g., by calling die_errno), they
may report a nonsense value.

Let's fix these sites by having them distinguish between the
two error cases. That avoids the random errno confusion, and
lets us give more detailed error messages.

Signed-off-by: Jeff King 
---
 pack-write.c |  7 ++-
 sha1_file.c  | 11 ---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index a333ec6754..fea6284192 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -213,14 +213,19 @@ void fixup_pack_header_footer(int pack_fd,
git_SHA_CTX old_sha1_ctx, new_sha1_ctx;
struct pack_header hdr;
char *buf;
+   ssize_t read_result;
 
git_SHA1_Init(_sha1_ctx);
git_SHA1_Init(_sha1_ctx);
 
if (lseek(pack_fd, 0, SEEK_SET) != 0)
die_errno("Failed seeking to start of '%s'", pack_name);
-   if (read_in_full(pack_fd, , sizeof(hdr)) != sizeof(hdr))
+   read_result = read_in_full(pack_fd, , sizeof(hdr));
+   if (read_result < 0)
die_errno("Unable to reread header of '%s'", pack_name);
+   else if (read_result != sizeof(hdr))
+   die_errno("Unexpected short read for header of '%s'",
+ pack_name);
if (lseek(pack_fd, 0, SEEK_SET) != 0)
die_errno("Failed seeking to start of '%s'", pack_name);
git_SHA1_Update(_sha1_ctx, , sizeof(hdr));
diff --git a/sha1_file.c b/sha1_file.c
index 5a2014811f..09ad64ce55 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1748,10 +1748,15 @@ static int index_core(unsigned char *sha1, int fd, 
size_t size,
ret = index_mem(sha1, "", size, type, path, flags);
} else if (size <= SMALL_FILE_SIZE) {
char *buf = xmalloc(size);
-   if (size == read_in_full(fd, buf, size))
-   ret = index_mem(sha1, buf, size, type, path, flags);
+   ssize_t read_result = read_in_full(fd, buf, size);
+   if (read_result < 0)
+   ret = error_errno("read error while indexing %s",
+ path ? path : "");
+   else if (read_result != size)
+   ret = error("short read while indexing %s",
+   path ? path : "");
else
-   ret = error_errno("short read");
+   ret = index_mem(sha1, buf, size, type, path, flags);
free(buf);
} else {
void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-- 
2.14.2.988.g01c8b37dde