Re: [PATCH] strbuf_read_file(): preserve errno across close() call

From: René Scharfe <l.s.r@web.de>
Date: Feb 23, 2018, 21:00 UTC
Message-ID: <f134c6bf-c147-4201-1217-b59bfb9f2288@web.de>
In-Reply-To: <20180223070053.GC19791@sigill.intra.peff.net>

Am 23.02.2018 um 08:00 schrieb Jeff King:

Show 7 quoted lines

> On Fri, Feb 23, 2018 at 01:49:52AM -0500, Jeff King wrote:
> Subject: [PATCH] strbuf_read_file(): preserve errno across close() call
> 
> If we encounter a read error, the user may want to report it
> by looking at errno. However, our close() call may clobber
> errno, leading to confusing results. Let's save and restore
> it in the error case.

Good idea.

Show 29 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   strbuf.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/strbuf.c b/strbuf.c
> index 1df674e919..5f138ed3c8 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -612,14 +612,18 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
>   {
>   	int fd;
>   	ssize_t len;
> +	int saved_errno;
>   
>   	fd = open(path, O_RDONLY);
>   	if (fd < 0)
>   		return -1;
>   	len = strbuf_read(sb, fd, hint);
> +	saved_errno = errno;
>   	close(fd);
> -	if (len < 0)
> +	if (len < 0) {
> +		errno = saved_errno;
>   		return -1;
> +	}
>   
>   	return len;
>   }

How about adding a stealthy close_no_errno(), or do something like the following to get shorter and more readable code? (We could also keep a single close() call, but would then set errno even on success.)

--- 
 strbuf.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 1df674e919..c0066b1db9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -2,6 +2,8 @@
 #include "refs.h"
 #include "utf8.h"
 
+#define IGNORE_ERROR(expr) do { int e_ = errno; expr; errno = e_; } while (0)
+
 int starts_with(const char *str, const char *prefix)
 {
 	for (; ; str++, prefix++)
@@ -391,7 +393,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 		if (got < 0) {
 			if (oldalloc == 0)
-				strbuf_release(sb);
+				IGNORE_ERROR(strbuf_release(sb));
 			else
 				strbuf_setlen(sb, oldlen);
 			return -1;
@@ -617,9 +619,11 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 	if (fd < 0)
 		return -1;
 	len = strbuf_read(sb, fd, hint);
-	close(fd);
-	if (len < 0)
+	if (len < 0) {
+		IGNORE_ERROR(close(fd));
 		return -1;
+	}
+	close(fd);
 
 	return len;
 }