From: Johannes Sixt <j.sixt@viscovery.net>
Date: Thu, 14 Feb 2008 08:00:28 GMT
Subject: Re: [PATCH 2/6] Automatically close stderr pipes created by run_command
Message-ID: <47B3F51C.3060002@viscovery.net>
In-Reply-To: <20080214062229.GB30516@spearce.org>

Shawn O. Pearce schrieb:
> Like the out pipe and in pipe, we now automatically close the err
> pipe if it was requested by the caller and it hasn't been closed
> by the caller.  This simplifies anyone who wants to get a pipe to
> the stderr stream for another process.

IMHO, this is backwards.

The .in, .out, .err members of struct child_process serve two different
purposes:

1. Caller sets them to some fd > 0. This means:
   "Here is a readable (.in) or writable (.out, .err) fd; use it."

2. Caller sets them to -1: This means:
   "Create a pipe and give me the writable (.in) or readable (.out,
   .err) end of it back."

Notice that in a clean implementation:

- case 1. would imply that the fd is "given away", i.e.
  start_command/finish_command take ownership and close it;

- case 2. would imply that the caller takes ownership of the returned
  fd and has to close it.

The current implementation of start_command/finish_command as well as its
callers don't follow these rules (because they are not documented
anywhere). As a nasty side-effect we have double-closes in many places
because some callers close fds even though they are not supposed to do it.
(That's the reason why the close() calls in finish_command cannot check
for errors!)

I've a patch cooking that cleans up this mess. IMHO, your patch makes
things messier.

-- Hannes