Re: [msysGit] [PATCH/RFC 07/11] run-command: support input-fd

On Freitag, 27. November 2009, Erik Faye-Lund wrote:

On Thu, Nov 26, 2009 at 10:53 PM, Johannes Sixt [off-list ref] wrote:

quoted

On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:

quoted

@@ -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.)

quoted

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