Argparser variable arg option parsing fix/improvement. - #13570
Draft
brbzull0 wants to merge 9 commits into
Draft
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.
Contributor
Author
|
[approve ci centos] |
brbzull0
marked this pull request as ready for review
August 21, 2026 15:42
An option whose value is optional had to be declared as taking zero or more values, the only variable arity available, so it also consumed the positional arguments of its own command. That is why traffic_ctl rejected "config get -c FILE RECORD" with an error naming get, and why --cold only worked written last or as --cold=FILE. Add AT_MOST_ONE_ARG_N, the equivalent of nargs='?' in Python argparse which this parser imitates, and declare --cold with it. The count check for the --option=value form now asks is_variable_arg_num() rather than comparing against the sentinels, so a third sentinel is not mistaken for a literal argument count.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/tscore/ArgParser.cc:716
append_option_data()indexesargs[i][0]/args[i][1]when checking for--option=value. If an argument is an empty string or a single "-" (possible for positional args / values), this is out-of-bounds and undefined behavior. Consider switching to a prefix check that doesn’t index into the string.
if (args[i][0] == '-' && args[i][1] == '-' && args[i].find('=') != std::string::npos) {
Contributor
|
@brbzull0 does this need to be back ported to 10.2.x. If so, please mark the project. |
The comment claimed the command's positional arguments were left in place, but collection only stops at a token naming another option, so positional tokens are still taken as values. Say so, and point at AT_MOST_ONE_ARG_N for an option whose value is optional.
An option expecting a fixed number of values took whatever token followed it, so "traffic_ctl server debug enable -t -a" set the debug tags to the literal "-a" and wrote that to the running configuration. Apply the rule the variable-arity path already follows: a token naming another option of the same command is not a value, so the missing value is reported, and "--" still passes a value that starts with '-'.
brbzull0
marked this pull request as draft
August 27, 2026 14:38
added 2 commits
August 28, 2026 12:24
is_variable_arg_num() exempts AT_MOST_ONE_ARG_N from the count check for the --option=value form, so --cold=a --cold=b silently kept the first and dropped the second, where the fixed arity equivalent is a usage error.
A bare -c before a record takes the record as the file name, which leaves config get with no records and exits with a usage error rather than reading a file named for the record.
added 2 commits
August 28, 2026 13:18
The repetition check only counted the --option=value form, so "-c a -c b" silently kept the last file name, and mixing the two spellings put two values in an option that permits one. Fixed arity options keep their existing last-one-wins behaviour, which is a separate concern.
Option recognition stays off for the rest of a variable-length value list, so every later token becomes a value and any option written afterwards is swallowed. Neither the guide nor the traffic_ctl page said so, which made "--" look safe to use before other options.
brbzull0
force-pushed
the
argparser-variable-arg-option-parsing
branch
from
August 28, 2026 11:20
c21e8c8 to
68e5708
Compare
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.The third commit adds the arity that was missing entirely,
AT_MOST_ONE_ARG_N, theequivalent of
nargs='?'in the argparse this parser imitates. An option whose value isoptional had to be declared as taking zero or more values, so it also ate the positional
arguments of its own command:
config get -c FILE RECORDfailed with an error namingget, and--coldonly worked written last or as--cold=FILE. That has been thebehaviour since 10.0.
--coldis now declared with the new arity, and the count check forthe
--option=valueform asksis_variable_arg_num()instead of comparing against thesentinels.
Fixes: #13569