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

[Jeff King <peff@xxxxxxxx>]

On Fri, Feb 23, 2018 at 10:00:24PM +0100, René Scharfe wrote:

> 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.)
> [...]
> @@ -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;

I dunno, that may be crossing the line of "too magical".

I had envisioned something like:

diff --git a/strbuf.c b/strbuf.c
index 5f138ed3c8..0790dd7bcb 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -365,6 +365,14 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)
 	}
 }
 
+/* release, but preserve errno */
+static void strbuf_release_careful(struct strbuf *sb)
+{
+	int saved_errno = errno;
+	strbuf_release(sb);
+	errno = saved_errno;
+}
+
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
@@ -375,7 +383,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 	if (res > 0)
 		strbuf_setlen(sb, sb->len + res);
 	else if (oldalloc == 0)
-		strbuf_release(sb);
+		strbuf_release_careful(sb);
 	return res;
 }
 
@@ -391,7 +399,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 		if (got < 0) {
 			if (oldalloc == 0)
-				strbuf_release(sb);
+				strbuf_release_careful(sb);
 			else
 				strbuf_setlen(sb, oldlen);
 			return -1;
@@ -416,7 +424,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
 	if (cnt > 0)
 		strbuf_setlen(sb, sb->len + cnt);
 	else if (oldalloc == 0)
-		strbuf_release(sb);
+		strbuf_release_careful(sb);
 	return cnt;
 }
 
@@ -482,7 +490,7 @@ int strbuf_getcwd(struct strbuf *sb)
 			break;
 	}
 	if (oldalloc == 0)
-		strbuf_release(sb);
+		strbuf_release_careful(sb);
 	else
 		strbuf_reset(sb);
 	return -1;


but that solution is definitely very specific to these cases. I also had
a feeling I should be able to shove the "oldalloc" logic into the
helper, too, but there are too many different behaviors in the "else"
block.

-Peff