On Sun, Nov 26, 2017 at 2:38 PM, Max Kirillov <[email protected]> wrote:
> [...]
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.
> [...]
A few small comments below; with the possible exception of one,
probably none worth a re-roll...
> diff --git a/http-backend.c b/http-backend.c
> @@ -317,6 +317,54 @@ static ssize_t read_request(int fd, unsigned char **out)
> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char
> **out)
> +{
> + unsigned char *buf = NULL;
> + ssize_t cnt = 0;
> +
> + if (max_request_buffer < req_len) {
> + die("request was larger than our maximum size (%lu): %lu;"
> + " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> + max_request_buffer,
> + req_len);
Unsigned format conversion '%lu' doesn't seem correct for ssize_t.
> + }
> +
> + if (req_len <= 0) {
> + *out = NULL;
> + return 0;
> + }
> +
> + buf = xmalloc(req_len);
> + cnt = read_in_full(fd, buf, req_len);
> + if (cnt < 0) {
> + free(buf);
> + return -1;
> + } else {
> + *out = buf;
> + return cnt;
> + }
This could have been written:
if (cnt < 0) {
free(buf);
return -1;
}
*out = buf;
return cnt;
but not worth a re-roll.
> +}
> +
> +static ssize_t env_content_length(void)
The caller of this function doesn't care how the content length is
being determined -- whether it comes from an environment variable or
is computed some other way; it cares only about the result. Having
"env" in the name ties it to checking only the environment. A more
generic name, such as get_content_length(), would help to decouple the
API from the implementation.
Nevertheless, not worth a re-roll.
> +{
> + ssize_t val = -1;
> + const char *str = getenv("CONTENT_LENGTH");
> +
> + if (str && !git_parse_ssize_t(str, &val))
git_parse_ssize_t() does the right thing even when 'str' is NULL, so
this condition could be simplified (but not worth a re-roll and may
not improve clarity).
> + die("failed to parse CONTENT_LENGTH: %s", str);
> + return val;
> +}
> +
> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> + ssize_t req_len = env_content_length();
Grabbing and parsing the value from the environment variable is
effectively a one-liner, so env_content_length() could be dropped
altogether, and instead (taking advantage of git_parse_ssize_t()'s
proper NULL-handling):
if (!git_parse_ssize_t(getenv(...), &req_len))
die(...);
Not worth a re-roll.
> + if (req_len < 0)
> + return read_request_eof(fd, out);
> + else
> + return read_request_fixed_len(fd, req_len, out);
> +}
