Re: [PATCH 26/26] remote-curl: implement connect-half-duplex command
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jan 10, 2018, 17:57 UTC
- Message-ID
- <20180110095730.1daaa70d7c583c58b85142a3@google.com>
- In-Reply-To
- <20180103001828.205012-27-bmwill@google.com>
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"?
Show 15 quoted lines
> +{
> + 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.
Show 21 quoted lines
> + 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.
Show 24 quoted lines
> +{
> + 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().
Show 13 quoted lines
> + > + /* 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.
Show 40 quoted lines
> +
> + 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".