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

["René Scharfe" <l.s.r@web.de>]

Am 23.02.2018 um 23:17 schrieb Junio C Hamano:
> 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));

> 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*

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