Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error

On Thu, Apr 14, 2011 at 10:43:33PM +0200, Johannes Sixt wrote:

quoted

quoted

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.

quoted

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.

quoted

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