Jonathan Nieder <jrnie...@gmail.com> writes:
> Kicking off the reviews: ;-)
>
> Jonathan Nieder wrote:
>
>> --- a/http-backend.c
>> +++ b/http-backend.c
>> @@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t
>> req_len, unsigned char **o
>>
>> static ssize_t get_content_length(void)
> [...]
>> + /*
>> + * According to RFC 3875, an empty or missing
>> + * CONTENT_LENGTH means "no body", but RFC 3875
>> + * precedes HTTP/1.1 and chunked encoding. Apache and
>> + * its imitators leave CONTENT_LENGTH unset for
>
> Which imitators? Maybe this should just say "Apache leaves [...]".
I tend to agree; I do not mind amending the text while queuing.
>> + * chunked requests, for which we should use EOF to
>> + * detect the end of the request.
>> + */
>> + str = getenv("HTTP_TRANSFER_ENCODING");
>> + if (str && !strcmp(str, "chunked"))
>
> RFC 2616 says Transfer-Encoding is a list of transfer-codings applied,
> in the order that they were applied, and that "chunked" is always
> applied last. That means a transfer-encoding like
>
> Transfer-Encoding: identity chunked
>
> would be permitted, or e.g.
>
> Transfer-Encoding: gzip chunked
>
> Does that means we should be using a check like
>
> str && (!strcmp(str, "chunked") || ends_with(str, " chunked"))
>
> ?
Hmph, that's
"Transfer-Encoding" ":" 1#transfer-coding
where #rule is
#rule
A construct "#" is defined, similar to "*", for defining lists of
elements. The full form is "<n>#<m>element" indicating at least
<n> and at most <m> elements, each separated by one or more commas
(",") and OPTIONAL linear white space (LWS). This makes the usual
form of lists very easy; a rule such as
( *LWS element *( *LWS "," *LWS element ))
can be shown as
1#element
So
- you need to account for comma
- your LWS may not be a SP
if you want to handle gzipped stream coming in a chunked form, I
think.
Unless I am missing the rule in CGI spec that is used to transform
the value on the Transfer-Encoding header to HTTP_TRANSFER_ENCODING
environment variable, that is.
> That said, a quick search of codesearch.debian.net mostly finds
> examples using straight comparison, so maybe the patch is fine as-is.
I do not think we would mind terribly if we do not support
combinations like gzipped-and-then-chunked from day one. An in-code
NEEDSWORK comment that refers to the production in RFC 2616 Page 143
may not hurt, though.
Thanks.
