Re: [RFC PATCH v3 5/8] Support taking over transports

From: Shawn O. Pearce <spearce@spearce.org>
Date: Dec 7, 2009, 17:49 UTC
Message-ID: <20091207174947.GF17173@spearce.org>
In-Reply-To: <1260116931-16549-6-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:

Show 12 quoted lines

> @@ -109,9 +120,21 @@ static struct child_process *get_helper(struct transport *transport)
>  		die("Unable to run helper: git %s", helper->argv[0]);
>  	data->helper = helper;
>  
> +	/* Open the output as FILE* so strbuf_getline() can be used.
> +	   Do this with duped fd because fclose() will close the fd,
> +	   and stuff like disowning will require the fd to remain.
> +
> +	   Set the stream to unbuffered because some reads are critical
> +	   in sense that any overreading will cause deadlocks.
> +	*/
> +        duped = dup(helper->out);

Formatting error here, the line is indented wrong.

> +	if (duped < 0)
> +		die_errno("Can't dup helper output fd");
> +	data->out = xfdopen(duped, "r");
> +	setvbuf(data->out, NULL, _IONBF, 0);

I wonder if this is really a good idea. Most helpers actually use a lot of text based IO to communicate. Disabling buffering for those helpers to avoid overreading the advertisement from a connect is a problem.

Maybe we could leave buffering on, but use a handshake protocol with the helper during connect:

  (1) > "connect git-upload-pack\n"
  (2) < "\n"
  (3) > "begin\n"

During 2 we are still buffered, but the only content on the pipe should be the single blank line, so we pull that in and the FILE* buffer should be empty.

After writing message 2 the remote helper blocks reading for the "begin\n" message 3. This gives the transport-helper.c code time to switch off the FILE* and start using raw IO off the pipe before any data starts coming down the line.

It does mean that the helper may need to run unbuffered IO, but if the helper is only a smart helper supporting connect then this isn't really a problem. It can run buffered IO until connect is received, switch to unbuffered, and use unbuffered for the single "begin\n" message it has to consume before it starts writing output or reading input.

-- 
Shawn.