On 02/27, Jonathan Nieder wrote:
> Hi,
>
> Brandon Williams wrote:
>
> > Teach remote-curl the 'stateless-connect' command which is used to
> > establish a stateless connection with servers which support protocol
> > version 2. This allows remote-curl to act as a proxy, allowing the git
> > client to communicate natively with a remote end, simply using
> > remote-curl as a pass through to convert requests to http.
>
> Cool! I better look at the spec for that first.
>
> *looks at the previous patch*
>
> Oh, there is no documented spec. :/ I'll muddle through this instead,
> then.
I'll make sure to add one :)
>
> [...]
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -188,7 +188,10 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
> > heads->version = discover_version(&reader);
> > switch (heads->version) {
> > case protocol_v2:
> > - die("support for protocol v2 not implemented yet");
> > + /*
> > + * Do nothing. Client should run 'stateless-connect' and
> > + * request the refs themselves.
> > + */
> > break;
>
> This is the 'list' command, right? Since we expect the client to run
> 'stateless-connect' instead, can we make it error out?
Yes and no. Remote-curl will run this when trying to establish a
stateless-connection. If the response is v2 then this is a capability
list and not refs. So the capabilities will be dumped to the client and
they will be able to request the refs themselves at a later point. The
comment here is just misleading, so i'll make sure to fix it.
>
> [...]
> > @@ -1082,6 +1085,184 @@ static void parse_push(struct strbuf *buf)
> > free(specs);
> > }
> >
> > +struct proxy_state {
> > + char *service_name;
> > + char *service_url;
> > + struct curl_slist *headers;
> > + struct strbuf request_buffer;
> > + int in;
> > + int out;
> > + struct packet_reader reader;
> > + size_t pos;
> > + int seen_flush;
> > +};
>
> Can this have a comment describing what it is/does? It's not obvious
> to me at first glance.
>
> It doesn't have to go in a lot of detail since this is an internal
> implementation detail, but something saying e.g. that this represents
> a connection to an HTTP server (that's an example; I'm not saying
> that's what it represents :)) would help.
Always making new code have higher standards than the existing code ;)
Haha, I'll add a simple comment explaining it.
>
> > +
> > +static void proxy_state_init(struct proxy_state *p, const char *service_name,
> > + enum protocol_version version)
> [...]
> > +static void proxy_state_clear(struct proxy_state *p)
>
> Looks sensible.
>
> [...]
> > +static size_t proxy_in(char *buffer, size_t eltsize,
> > + size_t nmemb, void *userdata)
>
> Can this have a comment describing the interface? (E.g. does it read
> a single pkt_line? How is the caller expected to use it? Does it
> satisfy the interface of some callback?)
I'll add a comment that its used as a READFUNCTION callback for curl and
that it tries to copy over a packet-line at a time.
>
> libcurl's example https://curl.haxx.se/libcurl/c/ftpupload.html just
> calls this read_callback. Such a name plus a pointer to
> CURLOPT_READFUNCTION should do the trick; bonus points if the comment
> says what our implementation of the callback does.
>
> Is this about having peek ability?
No its just that Curl only requests a set about of data at a time so you
need to be able to buffer the data that can't be read yet.
>
> > + struct proxy_state *p = userdata;
> > + 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("unexpected EOF when reading from parent process");
> > + 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(buffer, p->request_buffer.buf + p->pos, avail);
> > + p->pos += avail;
> > + return avail;
>
> This is a number of bytes, but CURLOPT_READFUNCTION expects a number
> of items, fread-style. That is:
>
> if (avail < eltsize)
> ... handle somehow, maybe by reading in more? ...
>
> avail_memb = avail / eltsize;
> memcpy(buffer,
> p->request_buffer.buf + p->pos,
> st_mult(avail_memb, eltsize));
> p->pos += st_mult(avail_memb, eltsize);
> return avail_memb;
>
> But https://curl.haxx.se/libcurl/c/CURLOPT_READFUNCTION.html says
>
> Your function must then return the actual number of bytes that
> it stored in that memory area.
>
> Does this mean eltsize is always 1? This is super confusing...
>
> ... ok, a quick grep for fread_func in libcurl reveals that eltsize is
> indeed always 1. Can we add an assertion so we notice if that
> changes?
>
> if (eltsize != 1)
> BUG("curl read callback called with size = %zu != 1", eltsize);
> max = nmemb;
Yeah i can go ahead and do this. Just note that the v1 path uses logic
identical to this so it would be a problem there.
>
> [...]
> > +static size_t proxy_out(char *buffer, size_t eltsize,
> > + size_t nmemb, void *userdata)
> > +{
> > + size_t size = eltsize * nmemb;
> > + struct proxy_state *p = userdata;
> > +
> > + write_or_die(p->out, buffer, size);
> > + return size;
> > +}
>
> Nice. Same questions about st_mult or just asserting on eltsize apply
> here, too.
>
> [...]
> > +static int proxy_post(struct proxy_state *p)
>
> What does this function do? Can it get a brief comment?
Will do.
>
> > +{
> > + struct active_request_slot *slot;
> > + int err;
> > +
> > + 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, p->headers);
> > +
> > + /* 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;
> > +
> > + return err;
>
> HTTP_OK is 0 but kind of obscures that. How about something like the
> following?
>
> if (run_slot(slot, NULL))
> return -1;
> return 0;
>
> or
>
> if (run_slot(slot, NULL) != HTTP_OK)
> return -1;
> return 0;
>
> That way it's clearer that this always returns 0 or -1.
Sounds good.
>
> [...]
> > +static int stateless_connect(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 stateless 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;
>
> Interesting. I wonder if we can make remote-curl less smart and drive
> this more from the caller.
>
> E.g. if the caller could do a single stateless request, they could do:
>
> option git-protocol version=2
> stateless-request GET info/refs?service=git-upload-pack
> [pkt-lines, ending with a flush-pkt]
>
> The git-protocol option in this hypothetical example is the value to
> be passed in the Git-Protocol header.
>
> Then based on the response, the caller could decide to keep using
> stateless-request for further requests or fall back to "fetch".
>
> That way, if we implement some protocol v3, the remote helper would
> not have to be changed at all to handle it: the caller would instead
> make the new v3-format request and remote-curl would be able to oblige
> without knowing why they're doing it.
>
> What do you think?
I do see the draw for wanting this. I think a change like this requires
a lot more refactoring, simply because with the current setup the
fetch/ls-refs logic doesn't care that its talking through a
remote-helper where if we went down that route it would need to be aware
of that.
--
Brandon Williams
