From: Geert Bosch <bosch@adacore.com>
Date: Wed, 27 Jun 2007 13:54:57 GMT
Subject: Re: [PATCH] git-log: detect dup and fdopen failure
Message-ID: <EF53B249-8430-4700-81AE-B97FD49FB955@adacore.com>
In-Reply-To: <87r6nxo8iq.fsf_-_@rho.meyering.net>

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");
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...

   -Geert