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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2018, 22:17 UTC
Message-ID: <xmqqk1v3s64y.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <f134c6bf-c147-4201-1217-b59bfb9f2288@web.de>

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;
>  }