Re: [PATCH] git-log: detect dup and fdopen failure
- From
Jim Meyering <jim@meyering.net>
- Date
- Jun 27, 2007, 14:06 UTC
- Message-ID
- <87zm2lmr01.fsf@rho.meyering.net>
- In-Reply-To
- <EF53B249-8430-4700-81AE-B97FD49FB955@adacore.com>
Geert Bosch <bosch@adacore.com> wrote:
Show 20 quoted lines
> 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.
Show 14 quoted lines
> 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.