threads / patch / 47905

patchsequencer: factor out strbuf_read_file_or_whine()

Subject: [PATCH] sequencer: factor out strbuf_read_file_or_whine()

## tl;dr

8 messages between Feb 22, 2018 and Feb 26, 2018. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

René Scharfe· Feb 22, 2018, 19:29 UTC · lore

Reduce code duplication by factoring out a function that reads an entire file into a strbuf, or reports errors on stderr if something goes wrong.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
The difference to using strbuf_read_file() is more detailed error
messages for open(2) failures.  But I don't know if we need them -- or
under which circumstances reading todo files could fail anyway.  When
doing multiple rebases in parallel perhaps?
 sequencer.c | 74 +++++++++++++++++++++++--------------------------------------
 1 file changed, 28 insertions(+), 46 deletions(-)
Show changes to sequencer.c +28 −46
diff --git a/sequencer.c b/sequencer.c
index e9baaf59bd..e34334f0ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)
 	return count;
 }
 
+static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
+{
+	int fd;
+	ssize_t len;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return error_errno(_("could not open '%s'"), path);
+	len = strbuf_read(sb, fd, 0);
+	close(fd);
+	if (len < 0)
+		return error(_("could not read '%s'."), path);
+	return len;
+}
+
 static int read_populate_todo(struct todo_list *todo_list,
 			struct replay_opts *opts)
 {
 	struct stat st;
 	const char *todo_file = get_todo_path(opts);
-	int fd, res;
+	int res;
 
 	strbuf_reset(&todo_list->buf);
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0)
-		return error_errno(_("could not open '%s'"), todo_file);
-	if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
-		close(fd);
-		return error(_("could not read '%s'."), todo_file);
-	}
-	close(fd);
+	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
+		return -1;
 
 	res = stat(todo_file, &st);
 	if (res)
@@ -3151,20 +3160,13 @@ int check_todo_list(void)
 	struct strbuf todo_file = STRBUF_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
 	struct strbuf missing = STRBUF_INIT;
-	int advise_to_edit_todo = 0, res = 0, fd, i;
+	int advise_to_edit_todo = 0, res = 0, i;
 
 	strbuf_addstr(&todo_file, rebase_path_todo());
-	fd = open(todo_file.buf, O_RDONLY);
-	if (fd < 0) {
-		res = error_errno(_("could not open '%s'"), todo_file.buf);
-		goto leave_check;
-	}
-	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
-		close(fd);
-		res = error(_("could not read '%s'."), todo_file.buf);
+	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
+		res = -1;
 		goto leave_check;
 	}
-	close(fd);
 	advise_to_edit_todo = res =
 		parse_insn_buffer(todo_list.buf.buf, &todo_list);
 
@@ -3180,17 +3182,10 @@ int check_todo_list(void)
 
 	todo_list_release(&todo_list);
 	strbuf_addstr(&todo_file, ".backup");
-	fd = open(todo_file.buf, O_RDONLY);
-	if (fd < 0) {
-		res = error_errno(_("could not open '%s'"), todo_file.buf);
-		goto leave_check;
-	}
-	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
-		close(fd);
-		res = error(_("could not read '%s'."), todo_file.buf);
+	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
+		res = -1;
 		goto leave_check;
 	}
-	close(fd);
 	strbuf_release(&todo_file);
 	res = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);
 
@@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)
 	}
 	strbuf_release(&buf);
 
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0) {
-		return error_errno(_("could not open '%s'"), todo_file);
-	}
-	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
-		close(fd);
-		return error(_("could not read '%s'."), todo_file);
-	}
-	close(fd);
+	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
+		return -1;
 	if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
 		todo_list_release(&todo_list);
 		return -1;
@@ -3370,17 +3358,11 @@ int rearrange_squash(void)
 	const char *todo_file = rebase_path_todo();
 	struct todo_list todo_list = TODO_LIST_INIT;
 	struct hashmap subject2item;
-	int res = 0, rearranged = 0, *next, *tail, fd, i;
+	int res = 0, rearranged = 0, *next, *tail, i;
 	char **subjects;
 
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0)
-		return error_errno(_("could not open '%s'"), todo_file);
-	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
-		close(fd);
-		return error(_("could not read '%s'."), todo_file);
-	}
-	close(fd);
+	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
+		return -1;
 	if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
 		todo_list_release(&todo_list);
 		return -1;

-- 
2.16.2
Junio C Hamano· Feb 22, 2018, 20:57 UTC · re: René Scharfe · lore

Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()

René Scharfe <l.s.r@web.de> writes:
Show 36 quoted lines
> Reduce code duplication by factoring out a function that reads an entire
> file into a strbuf, or reports errors on stderr if something goes wrong.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> The difference to using strbuf_read_file() is more detailed error
> messages for open(2) failures.  But I don't know if we need them -- or
> under which circumstances reading todo files could fail anyway.  When
> doing multiple rebases in parallel perhaps?
>
>  sequencer.c | 74 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 28 insertions(+), 46 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e9baaf59bd..e34334f0ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)
>  	return count;
>  }
>  
> +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
> +{
> +	int fd;
> +	ssize_t len;
> +
> +	fd = open(path, O_RDONLY);
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), path);
> +	len = strbuf_read(sb, fd, 0);
> +	close(fd);
> +	if (len < 0)
> +		return error(_("could not read '%s'."), path);
> +	return len;
> +}
> +

This looks like a good granularity of a unit of independent work. The original we see below looks like it was written with scissors and glue ;-)

It appears to me that no topic in flight introduce more instances that need to be converted with a quick trial merge to 'pu', so I'll queue this forked at the tip of 'master'.

Thanks.
Show 105 quoted lines
>  static int read_populate_todo(struct todo_list *todo_list,
>  			struct replay_opts *opts)
>  {
>  	struct stat st;
>  	const char *todo_file = get_todo_path(opts);
> -	int fd, res;
> +	int res;
>  
>  	strbuf_reset(&todo_list->buf);
> -	fd = open(todo_file, O_RDONLY);
> -	if (fd < 0)
> -		return error_errno(_("could not open '%s'"), todo_file);
> -	if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
> -		close(fd);
> -		return error(_("could not read '%s'."), todo_file);
> -	}
> -	close(fd);
> +	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
> +		return -1;
>  
>  	res = stat(todo_file, &st);
>  	if (res)
> @@ -3151,20 +3160,13 @@ int check_todo_list(void)
>  	struct strbuf todo_file = STRBUF_INIT;
>  	struct todo_list todo_list = TODO_LIST_INIT;
>  	struct strbuf missing = STRBUF_INIT;
> -	int advise_to_edit_todo = 0, res = 0, fd, i;
> +	int advise_to_edit_todo = 0, res = 0, i;
>  
>  	strbuf_addstr(&todo_file, rebase_path_todo());
> -	fd = open(todo_file.buf, O_RDONLY);
> -	if (fd < 0) {
> -		res = error_errno(_("could not open '%s'"), todo_file.buf);
> -		goto leave_check;
> -	}
> -	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -		close(fd);
> -		res = error(_("could not read '%s'."), todo_file.buf);
> +	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
> +		res = -1;
>  		goto leave_check;
>  	}
> -	close(fd);
>  	advise_to_edit_todo = res =
>  		parse_insn_buffer(todo_list.buf.buf, &todo_list);
>  
> @@ -3180,17 +3182,10 @@ int check_todo_list(void)
>  
>  	todo_list_release(&todo_list);
>  	strbuf_addstr(&todo_file, ".backup");
> -	fd = open(todo_file.buf, O_RDONLY);
> -	if (fd < 0) {
> -		res = error_errno(_("could not open '%s'"), todo_file.buf);
> -		goto leave_check;
> -	}
> -	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -		close(fd);
> -		res = error(_("could not read '%s'."), todo_file.buf);
> +	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
> +		res = -1;
>  		goto leave_check;
>  	}
> -	close(fd);
>  	strbuf_release(&todo_file);
>  	res = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);
>  
> @@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)
>  	}
>  	strbuf_release(&buf);
>  
> -	fd = open(todo_file, O_RDONLY);
> -	if (fd < 0) {
> -		return error_errno(_("could not open '%s'"), todo_file);
> -	}
> -	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -		close(fd);
> -		return error(_("could not read '%s'."), todo_file);
> -	}
> -	close(fd);
> +	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
> +		return -1;
>  	if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
>  		todo_list_release(&todo_list);
>  		return -1;
> @@ -3370,17 +3358,11 @@ int rearrange_squash(void)
>  	const char *todo_file = rebase_path_todo();
>  	struct todo_list todo_list = TODO_LIST_INIT;
>  	struct hashmap subject2item;
> -	int res = 0, rearranged = 0, *next, *tail, fd, i;
> +	int res = 0, rearranged = 0, *next, *tail, i;
>  	char **subjects;
>  
> -	fd = open(todo_file, O_RDONLY);
> -	if (fd < 0)
> -		return error_errno(_("could not open '%s'"), todo_file);
> -	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -		close(fd);
> -		return error(_("could not read '%s'."), todo_file);
> -	}
> -	close(fd);
> +	if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
> +		return -1;
>  	if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
>  		todo_list_release(&todo_list);
>  		return -1;
Jeff King· Feb 23, 2018, 06:49 UTC · re: René Scharfe · lore

Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()

On Thu, Feb 22, 2018 at 08:29:25PM +0100, René Scharfe wrote:
Show 9 quoted lines
> Reduce code duplication by factoring out a function that reads an entire
> file into a strbuf, or reports errors on stderr if something goes wrong.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> The difference to using strbuf_read_file() is more detailed error
> messages for open(2) failures.  But I don't know if we need them -- or
> under which circumstances reading todo files could fail anyway.  When
> doing multiple rebases in parallel perhaps?

I'm fine with this patch, but FWIW I think reporting the result of strbuf_read_file with error_errno() would actually be an improvement. The errno values are generally sufficient to tell if the problem was in opening or reading, and then we'd get more information in the case of a failed read() call.

Thought note...
Show 22 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index e9baaf59bd..e34334f0ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)
>  	return count;
>  }
>  
> +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
> +{
> +	int fd;
> +	ssize_t len;
> +
> +	fd = open(path, O_RDONLY);
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s'"), path);
> +	len = strbuf_read(sb, fd, 0);
> +	close(fd);
> +	if (len < 0)
> +		return error(_("could not read '%s'."), path);
> +	return len;
> +}

If we were to use error_errno() in the second conditional here, we should take care not to clobber errno during the close(). I think strbuf_read_file() actually has the same problem, which might be worth fixing.

-Peff
Jeff King· Feb 23, 2018, 07:00 UTC · re: Jeff King · lore

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

On Fri, Feb 23, 2018 at 01:49:52AM -0500, Jeff King wrote:
Show 19 quoted lines
> > +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
> > +{
> > +	int fd;
> > +	ssize_t len;
> > +
> > +	fd = open(path, O_RDONLY);
> > +	if (fd < 0)
> > +		return error_errno(_("could not open '%s'"), path);
> > +	len = strbuf_read(sb, fd, 0);
> > +	close(fd);
> > +	if (len < 0)
> > +		return error(_("could not read '%s'."), path);
> > +	return len;
> > +}
> 
> If we were to use error_errno() in the second conditional here, we
> should take care not to clobber errno during the close(). I think
> strbuf_read_file() actually has the same problem, which might be worth
> fixing.
Here's a patch, while I'm thinking about it.

I notice that quite a few strbuf error paths may call strbuf_release(), too. Technically free() may clobber errno, too. I don't know if it's worth protecting against (IIRC POSIX is being amended to disallow this, but I have no idea how common it is in existing platforms).

-- >8 --
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.

Signed-off-by: Jeff King <peff@peff.net>
---
 strbuf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to strbuf.c +5 −1
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;
 }

-- 
2.16.2.580.g96c83ce8ea
René Scharfe· Feb 23, 2018, 21:00 UTC · re: Jeff King · lore

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

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(-)
Show changes to strbuf.c +7 −3
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;
 }
Junio C Hamano· Feb 23, 2018, 22:17 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
> +#define IGNORE_ERROR(expr) do { int e_ = errno; expr; errno = e_; } while (0)
The macro certainly is a cute idea, but ...
Show 9 quoted lines
> @@ -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;

... ideally, I would imagine that we wish we could write this hunk to something that expands to:

		if (got < 0) {
			do {
                                int e_ = errno;
                                if (oldalloc == 0)
                                        strbuf_release(sb);
                                else
                                        strbuf_setlen(sb, oldlen);
                                errno = e_;
			} while (0);
			return -1;

no? That is (1) we do not want to rely too much on knowing that strbuf_setlen() is very thin and does not touch errno, and hence (2) we want to mark not just a single expr but a block as "we know we got an error and errno from that error is more precious than what we do in this block to clean thihngs up".

Of course, a pair of macros
	#define IGNORE_ERROR_BEGIN do { int e_ = errno
	#define IGNORE_ERROR_END errno = e_; } while (0)

is probably the only way to do so in C, and that is already too ugly to live, so we cannot achieve the ideal.

So I dunno..
Show 14 quoted lines
> @@ -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;
>  }
René Scharfe· Feb 23, 2018, 22:55 UTC · re: Junio C Hamano · lore

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

Am 23.02.2018 um 23:17 schrieb Junio C Hamano:
Show 35 quoted lines
> René Scharfe <l.s.r@web.de> writes:
> 
>> +#define IGNORE_ERROR(expr) do { int e_ = errno; expr; errno = e_; } while (0)
> 
> The macro certainly is a cute idea, but ...
> 
>> @@ -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;
> 
> ... ideally, I would imagine that we wish we could write this hunk
> to something that expands to:
> 
> 		if (got < 0) {
> 			do {
>                                  int e_ = errno;
>                                  if (oldalloc == 0)
>                                          strbuf_release(sb);
>                                  else
>                                          strbuf_setlen(sb, oldlen);
>                                  errno = e_;
> 			} while (0);
> 			return -1;
> 
> no?  That is (1) we do not want to rely too much on knowing that
> strbuf_setlen() is very thin and does not touch errno, and hence (2)
> we want to mark not just a single expr but a block as "we know we
> got an error and errno from that error is more precious than what we
> do in this block to clean thihngs up".

Relying on that internal knowledge should be OK in strbuf.c, but in this specific example we could of course do:

			if (oldalloc == 0)
				IGNORE_ERROR(strbuf_release(sb));
			else
				IGNORE_ERROR(strbuf_setlen(sb, oldlen));

I guess ignoring errors of whole blocks is not that common, based on a quick search (git grep -W int.*_errno). And in such a case we could factor that code out into a separate function, if really needed. Or continue saving errno explicitly.

Compilers should be smart enough to avoid saving and restoring errno between multiple uses of that macro, e.g. code like this would only do it once, from what I saw when experimenting with the Compiler Explorer (https://godbolt.org/):

	IGNORE_ERROR(close(fd1));
	IGNORE_ERROR(close(fd2));
Show 9 quoted lines
> Of course, a pair of macros
> 
> 	#define IGNORE_ERROR_BEGIN do { int e_ = errno
> 	#define IGNORE_ERROR_END errno = e_; } while (0)
> 
> is probably the only way to do so in C, and that is already too ugly
> to live, so we cannot achieve the ideal.
> 
> So I dunno..
*shudder*
Show 15 quoted lines
> 
>> @@ -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;
>>   }
Jeff King· Feb 26, 2018, 09:04 UTC · re: René Scharfe · lore

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

On Fri, Feb 23, 2018 at 10:00:24PM +0100, René Scharfe wrote:
Show 13 quoted lines
> 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:
Show changes to strbuf.c +12 −5
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

← back to recent threads