On 01/10, Jonathan Tan wrote:
> On Tue, 2 Jan 2018 16:18:28 -0800
> Brandon Williams <bmw...@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
