git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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.

Previous: Geert BoschNext: Jim Meyering
Message 11 of 15 in “detect dup failure”
  1. detect dup failureJim Meyering, Jun 27, 2007
  2. Alex RiesenJun 27, 2007
  3. git-log: detect dup and fdopen failureJim Meyering, Jun 27, 2007
  4. Alex RiesenJun 27, 2007
  5. Jim MeyeringJun 27, 2007
  6. Alex RiesenJun 27, 2007
  7. Jim MeyeringJun 27, 2007
  8. Alex RiesenJun 27, 2007
  9. Jim MeyeringJun 27, 2007
  10. Geert BoschJun 27, 2007
  11. Jim MeyeringJun 27, 2007
  12. Jim MeyeringJun 27, 2007
  13. Linus TorvaldsJun 27, 2007
  14. Josh TriplettJun 28, 2007
  15. Shawn O. PearceJun 28, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.