From: Johannes Sixt <j6t@kdbg.org>
Date: Fri, 27 Nov 2009 20:14:13 GMT
Subject: Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd
Message-ID: <200911272114.13107.j6t@kdbg.org>
In-Reply-To: <40aa078e0911270639n1de36517w5fdf6ef38e931b19@mail.gmail.com>

On Freitag, 27. November 2009, Erik Faye-Lund wrote:
> On Thu, Nov 26, 2009 at 10:53 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
> >> @@ -327,7 +327,10 @@ int start_async(struct async *async)
> >>  {
> >>       int pipe_out[2];
> >>
> >> -     if (pipe(pipe_out) < 0)
> >> +     if (async->out) {
> >> +             pipe_out[0] = dup(async->out);
> >> +             pipe_out[1] = dup(async->out);
> >> +     } else if (pipe(pipe_out) < 0)
> >>               return error("cannot create pipe: %s", strerror(errno));
> >>       async->out = pipe_out[0];
> >
> > Hm. If async->out != 0:
> >
> >        pipe_out[0] = dup(async->out);
> >        async->out = pipe_out[0];
> >
> > This is confusing.
>
> What do you find confusing about it? The idea is to use a provided
> bi-directional fd instead of a pipe if async->out is non-zero. The
> currently defined rules for async is that async->out must be zero
> (since the structure should be zero-initialized).

It is just the code structure that is confusing. It should be

	if (async->out) {
		/* fd was provided */
		do all that is needed in this case
	} else {
		/* fd was requested */
		do all for this other case
	}
	/* nothing to do anymore here */

(Of course, this should only replace the part that is cited above, not the 
whole function.)

> > Moreover, you are assigning (a dup of) the same fd to the writable end.
> > This assumes a bi-directional channel. I don't yet know what I should
> > think about this (haven't studied the later patches, yet).
>
> Indeed it does. Do we want to extend it to support a set of
> unidirectional channels instead?

Yes, I think so. We could pass a regular int fd[2] array around with the clear 
definition that both can be closed independently, i.e. one must be a dup() of 
the other. struct async would also have such an array.

Speaking of dup(): The underlying function is DuplicateHandle(), and its 
documentation says:

"You should not use DuplicateHandle to duplicate handles to the following 
objects: ... o Sockets. ... use WSADuplicateSocket."

But then the docs of WSADuplicateSocket() talk only about duplicating a socket 
to a separate process. Perhaps DuplicateHandle() of a socket within the same 
process Just Works?

-- Hannes