| Message ID | YUE1fGZc1FuuyUNH@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | limit memory allocations for v2 servers | expand |
On Tue, Sep 14 2021, Jeff King wrote: > -static int is_valid_capability(const char *key) > +static int receive_client_capability(const char *key) > { > const char *value; > const struct protocol_capability *c = get_capability(key, &value); > > - return c && c->advertise(the_repository, NULL); > + if (!c || !c->advertise(the_repository, NULL)) > + return 0; > + > + if (c->receive) > + c->receive(the_repository, value); > + return 1; > } > I haven't actually run this yet (and need to zZzZ soon), but AFAICT at the end of this series you leave the existing advertise semantics of advertise() be (which is fine). I have this unsubmitted patch locally which you may or may not want to work into this. I considered splitting up the advertise() method as well, i.e. we could have a new "is_advertised" boolean callback, and then a "capability_string" or whatever. "server-option" and "object-info" never add anything, so they could leave that as NULL. But it's probably not worth it, just food for thought... -- >8 -- serve: document that "advertise" is called in two modes If we're being called with a non-NULL argument we're sending out the advertisement line, but if it's NULL we're actually in the middle of a request. So you can use the check for NULL to emit your own "die" on "return 0", like "wtf, I said I don't support command xyz, why are you calling it?", as opposed to the default "invalid command '%s'". Maybe nobody cares, and we can't assume that we're going from an advertisement to a command for the same request anyway (can we?). I.e. are we serving multiple clients? Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- serve.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/serve.c b/serve.c index aa8209f147e..b187ce26911 100644 --- a/serve.c +++ b/serve.c @@ -55,6 +55,12 @@ struct protocol_capability { * Optionally a value can be specified by adding it to 'value'. * If a value is added to 'value', the server will advertise this * capability as "<name>=<value>" instead of "<name>". + * + * When called with a NULL value we're past the advertisement + * itself, and are checking during a request whether we + * support this command. This can be checked and used used to + * e.g. emit a die("we support this, but don't have it + * enabled!") error. */ int (*advertise)(struct repository *r, struct strbuf *value);
On Wed, Sep 15, 2021 at 02:31:28AM +0200, Ævar Arnfjörð Bjarmason wrote: > I haven't actually run this yet (and need to zZzZ soon), but AFAICT at > the end of this series you leave the existing advertise semantics of > advertise() be (which is fine). I have this unsubmitted patch locally > which you may or may not want to work into this. > > I considered splitting up the advertise() method as well, i.e. we could > have a new "is_advertised" boolean callback, and then a > "capability_string" or whatever. "server-option" and "object-info" never > add anything, so they could leave that as NULL. > > But it's probably not worth it, just food for thought... Yes, I noticed the advertise() function is really doing double-duty here. Nothing changes with respect to it in my series, but I agree it's a bit confusing. The functions are simple enough that splitting up the two functions of advertise() might make sense. Likewise, your documentation seems reasonable. I'd prefer to punt on it for now, though, as my series isn't otherwise touching this function. I'm sure there are textual conflicts with what I'm doing here, since it's nearby, but I'd prefer to just build more cleanups on top later. -Peff
Jeff King <peff@peff.net> writes: > + /* > + * Function called when a client requests the capability as a > + * non-command. This may be NULL if the capability does nothing. > + * > + * For a capability of the form "foo=bar", the value string points to > + * the content after the "=" (i.e., "bar"). For simple capabilities > + * (just "foo"), it is NULL. > + */ > + void (*receive)(struct repository *r, const char *value); What does "as a non-command" mean? To put it another way, when a client requests the capability as a command, what does the receive method do differently? > @@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char ** > return NULL; > } > > -static int is_valid_capability(const char *key) > +static int receive_client_capability(const char *key) > { > const char *value; > const struct protocol_capability *c = get_capability(key, &value); > > - return c && c->advertise(the_repository, NULL); > + if (!c || !c->advertise(the_repository, NULL)) > + return 0; > + > + if (c->receive) > + c->receive(the_repository, value); > + return 1; > } > > static int parse_command(const char *key, struct protocol_capability **command) > @@ -262,7 +277,7 @@ static int process_request(void) > case PACKET_READ_NORMAL: > /* collect request; a sequence of keys and values */ The comment tentatively gets slightly stale here, but that will be corrected at the end, so it would be fine ;-) > if (parse_command(reader.line, &command) || > - is_valid_capability(reader.line)) > + receive_client_capability(reader.line)) > strvec_push(&keys, reader.line); > else > die("unknown capability '%s'", reader.line);
On Wed, Sep 15, 2021 at 09:41:28AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > + /* > > + * Function called when a client requests the capability as a > > + * non-command. This may be NULL if the capability does nothing. > > + * > > + * For a capability of the form "foo=bar", the value string points to > > + * the content after the "=" (i.e., "bar"). For simple capabilities > > + * (just "foo"), it is NULL. > > + */ > > + void (*receive)(struct repository *r, const char *value); > > What does "as a non-command" mean? To put it another way, when a > client requests the capability as a command, what does the receive > method do differently? For each entry in this capability table, clients can say: command=foo or just: foo The latter is a non-command. The "receive" function is not called at all if it is a "command". I think this is a bit more clear when read together with the existing function just above (which you can't see in the diff context): /* * Function called when a client requests the capability as a command. * Will be provided a struct packet_reader 'request' which it should * use to read the command specific part of the request. Every command * MUST read until a flush packet is seen before sending a response. * * This field should be NULL for capabilities which are not commands. */ int (*command)(struct repository *r, struct packet_reader *request); I guess these could define "as a command", but I think it's pretty clear in the context. > > static int parse_command(const char *key, struct protocol_capability **command) > > @@ -262,7 +277,7 @@ static int process_request(void) > > case PACKET_READ_NORMAL: > > /* collect request; a sequence of keys and values */ > > The comment tentatively gets slightly stale here, but that will be > corrected at the end, so it would be fine ;-) Hmm. I think it is not stale here, as we are still collecting the "keys" strvec. But it _does_ get stale by the end, when we stop doing so. I'm preparing a re-roll for the test issues Eric noted, so I'll drop that comment in the appropriate patch. -Peff
diff --git a/serve.c b/serve.c index 78a4e83554..a161189984 100644 --- a/serve.c +++ b/serve.c @@ -70,6 +70,16 @@ struct protocol_capability { * This field should be NULL for capabilities which are not commands. */ int (*command)(struct repository *r, struct packet_reader *request); + + /* + * Function called when a client requests the capability as a + * non-command. This may be NULL if the capability does nothing. + * + * For a capability of the form "foo=bar", the value string points to + * the content after the "=" (i.e., "bar"). For simple capabilities + * (just "foo"), it is NULL. + */ + void (*receive)(struct repository *r, const char *value); }; static struct protocol_capability capabilities[] = { @@ -164,12 +174,17 @@ static struct protocol_capability *get_capability(const char *key, const char ** return NULL; } -static int is_valid_capability(const char *key) +static int receive_client_capability(const char *key) { const char *value; const struct protocol_capability *c = get_capability(key, &value); - return c && c->advertise(the_repository, NULL); + if (!c || !c->advertise(the_repository, NULL)) + return 0; + + if (c->receive) + c->receive(the_repository, value); + return 1; } static int parse_command(const char *key, struct protocol_capability **command) @@ -262,7 +277,7 @@ static int process_request(void) case PACKET_READ_NORMAL: /* collect request; a sequence of keys and values */ if (parse_command(reader.line, &command) || - is_valid_capability(reader.line)) + receive_client_capability(reader.line)) strvec_push(&keys, reader.line); else die("unknown capability '%s'", reader.line);
We have a capabilities table that tells us what we should tell the client we are capable of, and what to do when a client gives us a particular command (e.g., "command=ls-refs"). But it doesn't tell us what to do when the client sends us back a capability (e.g., "object-format=sha256"). We just collect them all in a strvec and hope somebody can use them later. Instead, let's provide a function pointer in the table to act on these. This will eventually help us avoid collecting the strings, which will be more efficient and less prone to mischief. Using the new method is optional, which helps in two ways: - we can move existing capabilities over to this new system gradually in individual commits - some capabilities we don't actually do anything with anyway. For example, the client is free to say "agent=git/1.2.3" to us, but we do not act on the information in any way. Signed-off-by: Jeff King <peff@peff.net> --- serve.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)