Re: git-daemon problem

From: Junio C Hamano <junkio@cox.net>
Date: Jul 13, 2006, 05:44 UTC
Message-ID: <7vveq2ukho.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <E1G0kNV-0006LK-QW@moooo.ath.cx>

Matthias Lederhofer <matled@gmx.net> writes:

Show 15 quoted lines

> Junio C Hamano <junkio@cox.net> 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?