Argparser variable arg option parsing fix/improvement. - #13570
Open
brbzull0 wants to merge 2 commits into
Open
Conversation
added 2 commits
August 19, 2026 12:36
ArgParser options declared with MORE_THAN_ZERO_ARG_N or MORE_THAN_ONE_ARG_N collected every remaining token, so an option written after one of them was silently swallowed as a value and never parsed. Collection now stops at a token naming another option of the same command, "--" ends option recognition so a value can still start with '-', and only the range actually consumed is erased. Separately, the --option=value path took the name up to the first '=' but the value from the last one, truncating any value containing '='. That made --directive=key.sub=val unusable, since directive values are key=value pairs by definition. Fixes: apache#13569
The guard rejecting directive values that start with '-' existed only because variable-argument parsing swallowed any option written after -D. That no longer happens, so the guard can only fire for a value the caller passed deliberately, and its advice to place -D last is now wrong. A malformed value is reported by the directive format check instead. Require values for both -D and -d. Supplying either with no values built a request identical to a plain reload, silently widening a scoped reload to every handler. Also document that -D may appear anywhere among the options and can be combined with -d, which the previous note said was impossible.
Contributor
Author
|
[approve ci debian] |
Contributor
Author
|
[approve ci] |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two long-standing ts::ArgParser option-parsing defects that impacted traffic_ctl config reload (and other variable-argument options): variable-argument options no longer swallow subsequent options on the same command, -- now terminates option recognition so dash-prefixed values remain expressible, and --option=value parsing no longer truncates values containing embedded =.
Changes:
- Update
ArgParservariable-argument handling to stop consuming at the next registered option (and honor--), and fix--option=valuevalue extraction to split on the first=. - Remove the
-D“must be last” workaround behavior intraffic_ctl, and make empty-D/-dinvocations explicit errors instead of silently degrading to full reloads. - Add unit + gold tests covering the corrected parsing behaviors and update
traffic_ctldocumentation accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/tscore/ArgParser.cc |
Adjust variable-arg option collection logic, add registered-option detection, and fix --opt=... value splitting. |
include/tscore/ArgParser.h |
Declare new Command helpers used for the updated parsing behavior. |
src/tscore/unit_tests/test_ArgParser.cc |
Add unit coverage for variable-arg stop-at-next-option, -- handling, and embedded-= values. |
src/traffic_ctl/CtrlCommands.cc |
Remove old -D ordering guard; error out on empty -d / -D to prevent silent full reload behavior. |
tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py |
Add end-to-end CLI parsing coverage for traffic_ctl config reload -D / -d interactions and --. |
doc/appendices/command-line/traffic_ctl.en.rst |
Update operator guidance for -D/-d, including -- usage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+545
to
+548
| // Variable number of arguments. Stop collecting at a token that names another option | ||
| // of this command, so that following options and this command's own positional | ||
| // arguments are left in place for the caller. A "--" token ends option recognition, | ||
| // which is how a value that starts with '-' can be passed. |
Contributor
Author
|
[approve ci centos] |
brbzull0
marked this pull request as ready for review
August 21, 2026 15:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ArgParser options taking a variable number of values collected every remaining token, so
an option written after one was swallowed and never parsed:
config reload -D ip_allow.id=foo -t mytoklost the token, while the reverse order worked. Collection nowstops at a token naming another declared option of the same command, and
--ends optionrecognition so a dash-prefixed value stays expressible.
Also,
--option=valuetook the name up to the first=but the value from the last, so--directive=ip_allow.id=fooarrived asfoo.The second commit drops the
-D-must-be-last workaround in traffic_ctl, makes an empty-D/-dan error rather than a silent full reload, and corrects the docs.Fixes: #13569