On Tue, 2 Jan 2018 16:18:28 -0800
Brandon Williams <bmwill@xxxxxxxxxx> 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"?
> +{
> + 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.
> + 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.
> +{
> + 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().
> +
> + /* 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".
