Re: [PATCH 26/26] remote-curl: implement connect-half-duplex command

[Brandon Williams <bmwill@google.com>]

On 01/10, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:28 -0800
> Brandon Williams <bmwill@google.com> wrote:
> 
> > +static size_t proxy_in(void *ptr, size_t eltsize,
> > +		       size_t nmemb, void *buffer_)
> 
> OK, I managed to look at the Curl stuff in more detail.
> 
> I know that these parameter names are what remote_curl.c has been using
> for its callbacks, but I find them confusing (in particular, some Curl
> documentation rightly refer to the 1st parameter as a buffer, and the
> 4th parameter is actually userdata). Also, according to the Curl
> documentation, the type of the first parameter is "char *". Could we
> change the type of the first parameter to "char *", and the name of the
> fourth parameter either to "proxy_state_" or "userdata"?

Sounds good, I'll make the change.

> 
> > +{
> > +	size_t max = eltsize * nmemb;
> > +	struct proxy_state *p = buffer_;
> > +	size_t avail = p->request_buffer.len - p->pos;
> > +
> > +	if (!avail) {
> > +		if (p->seen_flush) {
> > +			p->seen_flush = 0;
> > +			return 0;
> > +		}
> > +
> > +		strbuf_reset(&p->request_buffer);
> > +		switch (packet_reader_read(&p->reader)) {
> > +		case PACKET_READ_EOF:
> > +			die("error reading request from parent process");
> 
> This should say "BUG:", I think. I'm not sure what the best way of
> explaining it is, but basically connect_half_duplex is supposed to
> ensure (by peeking) that there is no EOF when proxy_in() is called.

This wouldn't necessarily be a bug if the parent dies early for some
reason though right?

> 
> > +		case PACKET_READ_NORMAL:
> > +			packet_buf_write_len(&p->request_buffer, p->reader.line,
> > +					     p->reader.pktlen);
> > +			break;
> > +		case PACKET_READ_DELIM:
> > +			packet_buf_delim(&p->request_buffer);
> > +			break;
> > +		case PACKET_READ_FLUSH:
> > +			packet_buf_flush(&p->request_buffer);
> > +			p->seen_flush = 1;
> > +			break;
> > +		}
> > +		p->pos = 0;
> > +		avail = p->request_buffer.len;
> > +	}
> > +
> > +	if (max < avail)
> > +		avail = max;
> > +	memcpy(ptr, p->request_buffer.buf + p->pos, avail);
> > +	p->pos += avail;
> > +	return avail;
> 
> Thanks, this looks correct. I wish that the Curl API had a way for us to
> say "here are 4 more bytes, and that is all" instead of us having to
> make a note (p->seen_flush) to remember to return 0 on the next call,
> but that's the way it is.
> 
> > +}
> > +static size_t proxy_out(char *ptr, size_t eltsize,
> > +			size_t nmemb, void *buffer_)
> 
> Add a blank line before proxy_out. Also, same comment as proxy_in()
> about the function signature.

I'll change this function too.

> 
> > +{
> > +	size_t size = eltsize * nmemb;
> > +	struct proxy_state *p = buffer_;
> > +
> > +	write_or_die(p->out, ptr, size);
> > +	return size;
> > +}
> > +
> > +static int proxy_post(struct proxy_state *p)
> > +{
> > +	struct active_request_slot *slot;
> > +	struct curl_slist *headers = http_copy_default_headers();
> > +	int err;
> > +
> > +	headers = curl_slist_append(headers, p->hdr_content_type);
> > +	headers = curl_slist_append(headers, p->hdr_accept);
> > +	headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
> > +
> > +	slot = get_active_slot();
> > +
> > +	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
> > +	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
> > +	curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
> > +	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> 
> I looked at the Curl documentation for CURLOPT_HTTPHEADER and
> curl_easy_setopt doesn't consume the argument here (in fact, it asks us
> to keep "headers" around), so it might be possible to just generate the
> headers once in proxy_state_init().

Yeah I'll go ahead and do that, it'll make the post function a bit
cleaner too.

> 
> > +
> > +	/* Setup function to read request from client */
> > +	curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, proxy_in);
> > +	curl_easy_setopt(slot->curl, CURLOPT_READDATA, p);
> > +
> > +	/* Setup function to write server response to client */
> > +	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);
> > +	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);
> > +
> > +	err = run_slot(slot, NULL);
> > +
> > +	if (err != HTTP_OK)
> > +		err = -1;
> 
> This seems to mean that we cannot have two requests in flight at the
> same time even while there is no response (from the fact that we have a
> HTTP status code after returning from run_slot()).
> 
> I thought that git fetch over HTTP uses the two-requests-in-flight
> optimization that it also does over other protocols like SSH, but I see
> that that code path (fetch_git() in remote-curl.c) also uses run_slot()
> indirectly, so maybe my assumption is wrong. Anyway, this is outside the
> scope of this patch.
> 
> > +
> > +	curl_slist_free_all(headers);
> > +	return err;
> > +}
> > +
> > +static int connect_half_duplex(const char *service_name)
> > +{
> > +	struct discovery *discover;
> > +	struct proxy_state p;
> > +
> > +	/*
> > +	 * Run the info/refs request and see if the server supports protocol
> > +	 * v2.  If and only if the server supports v2 can we successfully
> > +	 * establish a half-duplex connection, otherwise we need to tell the
> > +	 * client to fallback to using other transport helper functions to
> > +	 * complete their request.
> > +	 */
> > +	discover = discover_refs(service_name, 0);
> > +	if (discover->version != protocol_v2) {
> > +		printf("fallback\n");
> > +		fflush(stdout);
> > +		return -1;
> > +	} else {
> > +		/* Half-Duplex Connection established */
> > +		printf("\n");
> > +		fflush(stdout);
> > +	}
> > +
> > +	proxy_state_init(&p, service_name);
> > +
> > +	/*
> > +	 * Dump the capability listing that we got from the server earlier
> > +	 * during the info/refs request.
> > +	 */
> > +	write_or_die(p.out, discover->buf, discover->len);
> > +
> > +	/* Peek the next packet line.  Until we see EOF keep sending POSTs */
> > +	while (packet_reader_peek(&p.reader) != PACKET_READ_EOF) {
> > +		if (proxy_post(&p)) {
> > +			/* We would have an err here */
> 
> Probably better to comment "Error message already printed by
> proxy_post".

-- 
Brandon Williams