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

Geert Bosch [off-list ref] wrote:

On Jun 27, 2007, at 09:02, Jim Meyering wrote:

quoted

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

quoted

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.