Re: [PATCH v6 11/12] http: read HTTP WWW-Authenticate response headers
On 2023-01-18 03:42, Ævar Arnfjörð Bjarmason wrote:
Show 29 quoted lines
>
> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
>
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
>> + strbuf_add(&buf, ptr, size);
>> +
>> + /* Strip the CRLF that should be present at the end of each field */
>> + strbuf_trim_trailing_newline(&buf);
>> +
>> + /* Start of a new WWW-Authenticate header */
>> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
>> + while (isspace(*val))
>> + val++;
>> +
>> + strvec_push(values, val);
>> + http_auth.header_is_last_match = 1;
>> + goto exit;
>> [...]
>> + if (http_auth.header_is_last_match && isspace(*buf.buf)) {
>> + /* Trim leading whitespace from this continuation hdr line. */
>> + strbuf_ltrim(&buf);
>
>
> The mixture of this isspace() loop and then strbuf_ltrim() seems odd,
> why not stick with the strbuf API?
>
> I.e. after skip_iprefix() strbuf_splice() the start of the string away,
> then use strbuf_ltrim() in the first "if" branch here?You mean like this?
size_t size = st_mult(eltsize, nmemb);
struct strvec *values = &http_auth.wwwauth_headers;
struct strbuf buf = STRBUF_INIT;
- const char *val;
/*
* Header lines may not come NULL-terminated from libcurl so we must@@ -216,11 +215,11 @@ static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
strbuf_trim_trailing_newline(&buf);
/* Start of a new WWW-Authenticate header */
- if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
- while (isspace(*val))
- val++;
+ if (istarts_with(buf.buf, "www-authenticate:")) {
+ strbuf_splice(&buf, 0, 17, NULL, 0);
+ strbuf_ltrim(&buf);
- strvec_push(values, val);
+ strvec_push(values, buf.buf);
http_auth.header_is_last_match = 1;
goto exit;
}
I don't particularly like this given we're now introducing the 'magic' number
17 that's the length of `www-authenticate:`, plus `strbuf_splice` is doing
a lot more work moving pieces of memory around rather than just producing
a new starting pointer to the start of the value (skipping leading whitespace).
> Likewise this is open-coding the "isspace" in strbuf_ltrim() for the
> second "if". Maybe run the strbuf_ltrim() unconditionally, save away the
> length before, and then:
>
> if (http_auth.header_is_last_match && prev_len != buf.len) { ...
>
> ?
The suggestion of trimming and comparing lengths just makes a piece of code
handling a little-known edge case less immediately obvious in its intent in
my opinion. The current implementation of "if starts with a single space"
matches the definition of continuation header lines, rather than re-piecing
together this from "why are we trimming and comparing lengths?"
Perf-wise the current implementation is only adding one extra `isspace`
call which we're potentially about to do in a loop inside of `strbuf_ltrim`
anyway. Plus, the common case will be a single space anyway.
Thanks,
Matthew
Matthew John Cheetham <mjcheetham@outlook.com>