compress; Fixes a regression in protocol parser#13107
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the compress plugin configuration parser where list-style directives could leave the remainder of the line to be re-tokenized by the outer parser loop, producing warnings like failed to interpret "br,gzip".
Changes:
- After parsing
supported-algorithms ..., clear the remainingline_viewso list tokens aren’t reinterpreted as top-level directives. - After parsing
compressible-status-code ..., clear the remainingline_viewfor the same reason.
bneradt
left a comment
There was a problem hiding this comment.
Looks correct. I wish these didn't have such similar names, with some consuming and some not:
void add_allow(swoc::TextView allow);
void add_compressible_content_type(swoc::TextView content_type);
void add_compressible_status_codes(swoc::TextView &status_codes);
// ...
void add_compression_algorithms(swoc::TextView &algorithms);
//...
void set_range_request(swoc::TextView token);But I suppose the pattern is that those which take multiple tokens take by reference and consume, while those that take a single token don't consume.
Anyway, it should at least fix the bug.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
plugins/compress/configuration.cc:202
add_compression_algorithms()now consumes the rest of the line viaextractFirstToken(line, isCommaOrSpace), but it will treat trailing inline comments as another token (e.g.supported-algorithms gzip, br # note) and fall into the "Unknown compression type" error path. Consider stopping parsing when the extracted token starts with#(consistent with the main parse loop’s comment handling).
HostConfiguration::add_compression_algorithms(swoc::TextView &line)
{
compression_algorithms_ = ALGORITHM_DEFAULT; // remove the default gzip.
for (;;) {
auto token = extractFirstToken(line, isCommaOrSpace);
if (token.empty()) {
plugins/compress/configuration.cc:236
add_compressible_status_codes()will currently attempt to parse trailing inline comments as a status code token (e.g.compressible-status-code 200, 206 # note), which triggers the "Invalid status code" error. Consider breaking out of the loop when the extracted token starts with#to match the parser’s comment semantics.
HostConfiguration::add_compressible_status_codes(swoc::TextView &line)
{
compressible_status_codes_.clear();
for (;;) {
auto token = extractFirstToken(line, isCommaOrSpace);
This is a regression I think, in 10.2, where we get warnings like ...(Parse)> (compress) WARNING: failed to interpret "br,gzip" at line 2 Also address copilots concerns
|
I tested the compress autest on my mac with current master (without these changes), and it fails the tests when zstd is available and built. I think (definitely not sure) that something in the combinations of the builds, and the real fixes here to the parser, ended up triggering failing these autests. So, although this code change doesn't change any behavior, other than fixing the parsing failures, it also surfaced errors in the autest gold files as well. |
|
Cherry-picked to 10.2.x |
* compress; Fixes a regression in protocol parser This is a regression I think, in 10.2, where we get warnings like ...(Parse)> (compress) WARNING: failed to interpret "br,gzip" at line 2 Also address copilots concerns * Fix failing autests, after our fixes. This was broken on master before too. (cherry picked from commit 450d3f3)
This is a regression I think, in 10.2, where we get warnings like
...(Parse)> (compress) WARNING: failed to interpret "br,gzip" at line 2