Matthias Lederhofer <matled@xxxxxxx> writes:
> Junio C Hamano <junkio@xxxxxxx> wrote:
>> This breaks the newer clients that knows how to do side-band
>> doesn't it?
> Probably, this patch is just to give a starting point where the
> problem could be.
>
>> Would this replacement patch help?
> [use write instead of safe_write]
>
> This seems to fix it. Perhaps it should be xwrite instead of write.
>
>> Maybe we should check if fd 2 is sane at daemon startup, and
>> otherwise open /dev/null for writing and dup2 it to fd 2?
> daemon startup is probably not the right place because as long as the
> terminal is open this will be fine.
Hmph. In the part of my message you did not quote:
$ git-daemon 0<&- 1>&- 2>&- --export-all /pub/git
leaves listening sockets at fd 0/1 without any fd 2, and
$ git-daemon 2>&- --export-all /pub/git
allocates listening socket at FD 2 (because FD 0 and FD 1 are
occupied).
Now, after we do accept(), we spawn a subprocess in handle(),
and in the child process dup2() the fd connected to the peer to
fd 0 and 1 of the child process -- and we do not do anything to
fd 2 of the child process. So in the latter case, my tentative
patch would write error message to the listening socket -- ugh.
And as you say, fd 2 might be connected to the terminal and
healthy when you start the daemon, but later you can close the
terminal, so there is no sane place for us to try anything
sensible.
The only "right" solution I could think of is to properly
daemonize git-daemon when not running under --inetd mode. Close
and open /dev/null the low three fds, and dissociate the process
from the controlling terminal (did I forget anything else --
perhaps chdir("/") at the top?). And we keep the current
behaviour of assuming the sane set of low three fds when a new
option --debug is given to help people look at its stderr. The
tentative patch to upload-pack would become moot at that point.
Hmm?
-
: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html