On Fri, Nov 27, 2009 at 3:23 PM, Erik Faye-Lund <kusmabite@xxxxxxxxxxxxxx> 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@xxxxxxxx> 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 -- To unsubscribe from this list: 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