On Thu, Apr 14, 2011 at 10:43:33PM +0200, Johannes Sixt wrote:
> > > In the threaded case, this fd is closed by start_command(), where it is
> > > passed as po.out in pack_objects(). In the fork case this is too late
> > > because a duplicate was already inherited to the sideband demuxer.
> >
> > Hrm, I see the code now. That seems like an odd thing to do to me.
>
> Why so? It's a matter of resource ownership: If you pass a positive value, you
> give away ownership; if you pass -1, you gain ownership; if you pass 0,
> ownership remains unchanged.
I can see how that is useful. Mostly I was just surprised, because I
wouldn't expect ownership to be transferred there.
> > Doesn't it disallow:
> >
> > /* set up a command */
> > const char **argv = { "some", "command" };
> > struct child_process c;
> > c.argv = argv;
> > c.out = fd;
> >
> > /* run it */
> > run_command(&c);
> >
> > /* now tack our own output to the end */
> > write(fd, "foo", 3);
>
> You would have to dup() the fd before run_command().
True. That makes it less of a big deal, because for the times that you
don't want full ownership transferred, you can work around it.
> > And even weirder, we only do the close for high file descriptors. So you
> > _can_ do that above if "fd" is stdout, but not with an arbitrary fd.
>
> Ah, right, that's a bit dubious. The reason is that if you want to tell the
> child process to use the parent's stdout for its own stdout, you specify 0
> aka "no special treatement", i.e. just inherit from the parent, not 1. IOW, 1
> is never a sane candidate to be assigned to c.out.
Fair enough.
So what do you want to do about the fd that needs closing? The options
I see are:
1. Try for a general solution. That probably means the "close every
descriptor in the child" hackiness that I mentioned earlier.
2. Fix this case by having the async code close it if it was forked.
It needs to know whether we forked, so we can:
a. Use NO_PTHREADS. Easy and simple, though it does break
start_async's abstraction a bit.
b. Have start_async pass in a flag telling what happened. This
really breaks the abstraction very similarly to (a), but it
makes the connection more explicit.
I think I am leaning a bit towards (2a). It's simple, and it's not like
this is library code with a million unknown callers; fixing it simply
and cleanly with a nice commit message is probably sufficient.
-Peff
--
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