Re: [msysGit] [PATCH/RFC 08/11] daemon: use explicit file descriptor

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Fri, Nov 27, 2009 at 3:23 PM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> Sorry for the long delay in the reply, but I'm a little low on time
> these days (and I've already spent some time trying to figure out what
> I was thinking - I wrote these patches a while ago).
>
> On Thu, Nov 26, 2009 at 11:03 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>>> @@ -372,37 +372,35 @@ static int run_service_command(const char **argv)
>>>       cld.argv = argv;
>>>       cld.git_cmd = 1;
>>>       cld.err = -1;
>>> +     cld.in = cld.out = fd;
>>
>> You shouldn't do that. In fact, the next patch 9 has a hunk that correctly
>> calls dup() once.
>>
>
> OK, as long as it works as expected, sure. But perhaps this needs a
> little change (see discussion later)
>
>>> -     close(0);
>>> -     close(1);
>>
>> Here, stdin and stdout were closed and start_command() used both. But these
>> two new calls
>>
>>> +     exit(execute(0, addr));
>>> ...
>>> +             return execute(0, peer);
>>
>> are the only places where a value is assigned to fd. Now it is always only
>> stdin. Where does the old code initialize stdout? Shouldn't this place need a
>> change, too?
>
> The "dup2(incoming, 0)"-call in handle() is AFAICT what makes it work
> to use the forked process' stdin as both stdin and stdout for the
> service-process pipe (since fd 0 now becomes a pipe that is both
> readable and writable). This isn't exactly a pretty mechanism, and I
> guess I should rework it. At the very least, I should remove the
> "dup2(incoming, 1)"-call, but I'm open to other suggestions. Perhaps I
> can change this patch to do the entire socket-passing (which is
> currently in the next patch)?
>

Something along these lines?

---8<---
diff --git a/daemon.c b/daemon.c
index a0aead5..8774ed5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -372,7 +372,8 @@ static int run_service_command(int fd, const char **argv)
 	cld.argv = argv;
 	cld.git_cmd = 1;
 	cld.err = -1;
-	cld.in = cld.out = fd;
+	cld.in = dup(fd);
+	cld.out = fd;
 	if (start_command(&cld))
 		return -1;

@@ -707,11 +708,7 @@ static void handle(int incoming, struct sockaddr
*addr, int addrlen)
 		return;
 	}

-	dup2(incoming, 0);
-	dup2(incoming, 1);
-	close(incoming);
-
-	exit(execute(0, addr));
+	exit(execute(incoming, addr));
 }

 static void child_handler(int signo)
---8<---

When I think more about it, I might've broken the inetd-mode as it
should communicate over stdin and stdout (not just stdin as it would
try to do now)... I don't know the inetd internals, but this frightens
me a bit.

So perhaps I'll need to provide support for two unidirectional file
descriptors after all?

-- 
Erik "kusma" Faye-Lund