Re: [PATCH] git-log: detect dup and fdopen failure

[Jim Meyering <jim@meyering.net>]

Geert Bosch <bosch@adacore.com> wrote:

> On Jun 27, 2007, at 09:02, Jim Meyering wrote:
>> -	if (!use_stdout)
>> -		realstdout = fdopen(dup(1), "w");
>> +	if (!use_stdout) {
>> +		int fd = dup(1);
>> +		if (fd < 0 || (realstdout = fdopen(fd, "w")) == NULL)
>> +			die("failed to duplicate standard output: %s",
>> +			    strerror(errno));
>> +	}
>
> This makes the code unreadable! A great way to ruin
> perfectly fine code is to add tons of error checking.
> The error checking is likely wrong (detects non-errors,
> or fails to detect real ones), and for sure makes code
> untestable  and unreadable.
>
> If we really case about catching such errors, write
> the code as:
> 	if (!use_stdout)
> 		realstdout = xfdopen(dup(1), "w");

Of course.  That is much more readable.
Though, perhaps you meant this?

> 		realstdout = xfdopen(xdup(1), "w");

If so, I'll be happy to prepare a patch to do that instead.
IMHO, we should never ignore syscall errors, and when preparing a
patch for a project like git (to which I haven't contributed much yet),
I prefer to keep the initial patch small and localized.

> where xfdopen is a wrapper around fdopen that dies in
> case of an error. This follows a practice we use elsewhere,
> and only adds one character to the code and only affects
> readability very slightly.
>
>> Without this, if you ever run out of file descriptors, dup will
>> fail (silently), fdopen will return NULL, and fprintf will
>> try to dereference NULL (i.e., usually segfault).
>
> As it is unlikely the failure mode will ever occur in practice,
> any way of aborting is fine. Even SIGSEGV would do: it would be
> trivial to find that we were leaking file descriptors or are out
> of memory. Oh, wait, that means we don't need any checking code
> at all...

Aren't you presuming the problem is easily reproducible
(and encountered by someone capable of investigating/reproducing),
or maybe that the abort left a usable core dump behind?

In my experience, the hardest bugs to track down are those
that are very rare and hard to reproduce.