From 298b5fd72781f409b6297dda8426ace9c5f15b0f Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 19 Aug 2026 12:36:55 +0200 Subject: [PATCH 1/9] Stop variable-arg options consuming later options 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: #13569 --- include/tscore/ArgParser.h | 4 ++ src/tscore/ArgParser.cc | 51 ++++++++++++++---- src/tscore/unit_tests/test_ArgParser.cc | 71 +++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 9 deletions(-) diff --git a/include/tscore/ArgParser.h b/include/tscore/ArgParser.h index fdb6086eba0..57a2c0b6541 100644 --- a/include/tscore/ArgParser.h +++ b/include/tscore/ArgParser.h @@ -222,6 +222,10 @@ class ArgParser void version_message() const; // Helper method for parse() void append_option_data(Arguments &ret, AP_StrVec &args, int index); + // Helper method to collect the values of an option or command into @a ret + std::string handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const; + // Whether @a token names an option registered on this command + bool is_registered_option(std::string const &token) const; // Helper method to validate mutually exclusive groups void validate_mutex_groups(Arguments &ret) const; // Helper method to validate option dependencies diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 90c3c2f5e43..9c49f51c8e4 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -518,23 +518,56 @@ ArgParser::Command::output_option() const } } +bool +ArgParser::Command::is_registered_option(std::string const &token) const +{ + if (_option_list.find(token) != _option_list.end() || _option_map.find(token) != _option_map.end()) { + return true; + } + // The --option=value form. + if (token.size() > 2 && token[0] == '-' && token[1] == '-') { + if (auto const pos = token.find_first_of('='); pos != std::string::npos) { + return _option_list.find(token.substr(0, pos)) != _option_list.end(); + } + } + return false; +} + // helper method to handle the arguments and put them nicely in arguments // can be switched to ts::errata -static std::string -handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) +std::string +ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const { ArgumentData data; ret.append(name, data); // handle the args if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) { - // infinite arguments - if (arg_num == MORE_THAN_ONE_ARG_N && args.size() <= index + 1) { - return "at least one argument expected by " + name; - } - for (unsigned j = index + 1; j < args.size(); j++) { + // 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. + unsigned j{index + 1}; + unsigned collected{0}; + bool recognize_options{true}; + + for (; j < args.size(); j++) { + if (recognize_options) { + if (args[j] == "--") { + recognize_options = false; + continue; + } + if (is_registered_option(args[j])) { + break; + } + } ret.append_arg(name, args[j]); + ++collected; + } + if (arg_num == MORE_THAN_ONE_ARG_N && collected == 0) { + return "at least one argument expected by " + name; } - args.erase(args.begin() + index, args.end()); + args.erase(args.begin() + index, args.begin() + j); + index -= 1; return ""; } // finite number of argument handling @@ -658,7 +691,7 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde if (args[i][0] == '-' && args[i][1] == '-' && args[i].find('=') != std::string::npos) { // deal with --args= std::string option_name = args[i].substr(0, args[i].find_first_of('=')); - std::string value = args[i].substr(args[i].find_last_of('=') + 1); + std::string value = args[i].substr(args[i].find_first_of('=') + 1); if (value.empty()) { help_message("missing argument for '" + option_name + "'"); } diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index 0a32502e964..a3117e57e57 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -217,3 +217,74 @@ TEST_CASE("with_required does not trigger on default values", "[parse]") REQUIRE(parsed.get("threshold").value() == "300"); REQUIRE(parsed.get("verbose") == true); } + +TEST_CASE("Variable argument option stops at a following option", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--token", "-t", "a token", "", 1, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // A flag after a variable argument option is not swallowed as a value. + const char *argv1[] = {"test_prog", "reload", "-D", "a.id=1", "b.id=2", "-m", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + REQUIRE(parsed.get("monitor") == true); + + // A following option keeps its own argument. + const char *argv2[] = {"test_prog", "reload", "-D", "a.id=1", "-t", "my_token", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("token").value() == "my_token"); + + // The long form of the following option is recognized too. + const char *argv3[] = {"test_prog", "reload", "-D", "a.id=1", "--monitor", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("monitor") == true); + + // So is its --option=value form. + const char *argv4[] = {"test_prog", "reload", "-D", "a.id=1", "--token=my_token", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("token").value() == "my_token"); +} + +TEST_CASE("Double dash ends option recognition for variable argument options", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // After "--" a token that looks like an option is taken as a value instead. + const char *argv[] = {"test_prog", "reload", "-D", "--", "-m", "a.id=1", nullptr}; + ts::Arguments parsed = parser.parse(argv); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "-m"); + REQUIRE(parsed.get("directive")[1] == "a.id=1"); + REQUIRE(parsed.get("monitor") == false); +} + +TEST_CASE("Option value keeps embedded equal signs", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + + // Only the first '=' separates the option from its value. + const char *argv[] = {"test_prog", "reload", "--directive=ip_allow.id=foo", nullptr}; + ts::Arguments parsed = parser.parse(argv); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("directive")[0] == "ip_allow.id=foo"); +} From 2c5f30d63b714ddd85c01361390f152e22ae0feb Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 19 Aug 2026 13:06:41 +0200 Subject: [PATCH 2/9] Drop the -D placement workaround from traffic_ctl 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. --- .../command-line/traffic_ctl.en.rst | 16 ++- src/traffic_ctl/CtrlCommands.cc | 20 +-- .../config_reload_directive_cli.test.py | 119 ++++++++++++++++++ 3 files changed, 143 insertions(+), 12 deletions(-) create mode 100644 tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 67ba5801349..298cfe7193c 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -456,11 +456,17 @@ Display the current value of a configuration record. .. note:: - ``-D`` uses variable-argument parsing and must appear as the **last option** - on the command line. Any flags placed after ``-D`` will be consumed as directive - values. ``-D`` and ``-d`` cannot be combined in the same invocation due to this - same constraint. Use ``-d`` with full YAML when you need both directives and - inline content in a single reload request. + ``-D`` accepts values until the next option or the end of the command line, so it + may appear anywhere among the options and can be combined with ``-d`` — directives + and inline content merge under the same config key: + + .. code-block:: bash + + $ traffic_ctl config reload -D myconfig.id=foo --monitor + $ traffic_ctl config reload -D myconfig.id=foo -d 'myconfig: {rules: [a]}' + + To pass a directive value that begins with ``-``, place ``--`` before it; every + token after ``--`` is taken as a value rather than an option. .. note:: diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 88d29637ece..c4d539251bc 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -555,6 +555,14 @@ ConfigCommand::config_reload() _printer->write_output(""); } + // Without content the request would silently degrade to a full reload of every handler, + // which is the opposite of the scoped reload the operator asked for. + if (data_args && data_args.size() == 0) { + _printer->write_output("Error: --data (-d) requires content: @file, @- or a YAML string"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + // Parse inline config data if provided (supports multiple -d arguments) YAML::Node configs; for (auto const &data_arg : data_args) { @@ -587,17 +595,15 @@ ConfigCommand::config_reload() // Parse --directive (-D) arguments into configs[key]["_reload"][directive] = value auto dir_args = get_parsed_arguments()->get("directive"); + if (dir_args && dir_args.size() == 0) { + _printer->write_output("Error: --directive (-D) requires at least one config_key.directive_key=value"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } for (auto const &dir : dir_args) { if (dir.empty()) { continue; } - if (dir[0] == '-') { - _printer->write_output("Error: '" + dir + - "' looks like a flag, not a directive. " - "Place -D as the last option on the command line."); - App_Exit_Status_Code = CTRL_EX_ERROR; - return; - } std::string err; if (!parse_directive(dir, configs, err)) { _printer->write_output("Error: " + err); diff --git a/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py new file mode 100644 index 00000000000..b73efcb8562 --- /dev/null +++ b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py @@ -0,0 +1,119 @@ +''' +Verify traffic_ctl command line parsing for the reload options that take a +variable number of values, --directive (-D) and --data (-d). + +Options declared with MORE_THAN_ZERO_ARG_N used to consume every remaining +token, so any option written after -D was silently swallowed as a directive +value and never parsed. -D therefore had to be the last option, and -D could +not be combined with -d. These runs assert on the JSONRPC request that +traffic_ctl builds (printed by -f rpc), because the subject under test is the +command line parsing rather than the server side handling of the reload. +''' +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +Test.Summary = 'Verify traffic_ctl -D/-d argument parsing for config reload' +Test.ContinueOnFail = True + +ts = Test.MakeATSProcess("ts") +ts.StartupTimeout = 30 + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rpc|config.reload', +}) + +ts.Disk.ip_allow_yaml.AddLines([ + 'ip_allow:', + '- apply: in', + ' ip_addrs: 0/0', + ' action: allow', + ' methods: ALL', +]) + +# ============================================================================ +# Test 1: an option written after -D keeps its own argument +# ============================================================================ +tr = Test.AddTestRun("Option after -D is not consumed as a directive value") +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=foo -t cli_token_1 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_1"', "-t must survive after -D") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"id": "foo"', "the directive must still be parsed") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 2: several directives, then an option +# ============================================================================ +tr = Test.AddTestRun("Multiple directives followed by an option") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=1 sni.id=2 -t cli_token_2 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_2"', "-t must survive after -D") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "first directive key must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "second directive key must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 3: -D combined with -d, which the parser previously made impossible +# ============================================================================ +tr = Test.AddTestRun("-D can be combined with -d") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=foo -d 'ip_allow: {rules: [x]}' -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"rules"', "inline content from -d must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"_reload"', "directives from -D must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 4: --directive=value keeps a value that itself contains '=' +# ============================================================================ +tr = Test.AddTestRun("--directive=value preserves embedded equal signs") +tr.Processes.Default.Command = "traffic_ctl config reload --directive=ip_allow.id=foo -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"id": "foo"', "the whole value must reach the request") +tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression("Invalid directive format", "the value must parse cleanly") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 5: "--" ends option recognition, so the value is taken literally and +# then rejected by the directive format check +# ============================================================================ +tr = Test.AddTestRun("A value after -- is taken literally") +tr.Processes.Default.Command = "traffic_ctl config reload -D -- -m" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "Invalid directive format '-m'", "-m must be treated as a directive value, not as --monitor") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 6: -D without any directive would silently reload every handler +# ============================================================================ +tr = Test.AddTestRun("-D requires at least one directive") +tr.Processes.Default.Command = "traffic_ctl config reload -D" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression("requires at least one", "-D must not be a silent no-op") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 7: same for -d, where a silent full reload is especially misleading +# ============================================================================ +tr = Test.AddTestRun("-d requires content") +tr.Processes.Default.Command = "traffic_ctl config reload -d" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression("requires content", "-d must not be a silent no-op") +tr.StillRunningAfter = ts From d8520e0e6103063f64f9dc367be01424a35f9311 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Mon, 24 Aug 2026 13:15:49 +0200 Subject: [PATCH 3/9] Add an at-most-one-argument arity to ArgParser 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. --- .../command-line/traffic_ctl.en.rst | 19 +++++ .../internal-libraries/ArgParser.en.rst | 23 ++++++ include/tscore/ArgParser.h | 13 ++++ src/traffic_ctl/traffic_ctl.cc | 4 +- src/tscore/ArgParser.cc | 27 ++++++- src/tscore/unit_tests/test_ArgParser.cc | 70 +++++++++++++++++++ .../records/traffic_ctl_cold_config.test.py | 40 +++++++++++ 7 files changed, 193 insertions(+), 3 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 298cfe7193c..f0f2aeeea6c 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -569,6 +569,25 @@ Display the current value of a configuration record. Specifying the file name is not needed as `traffic_ctl` will try to use the build(or the runroot if used) information to figure out the path to the `records.yaml`. + ``-c`` accepts at most one file name, so it may be written before or after the record + names: + + .. code-block:: bash + + $ traffic_ctl config get -c records.yaml proxy.config.diags.debug.enabled + $ traffic_ctl config get proxy.config.diags.debug.enabled -c records.yaml + $ traffic_ctl config get --cold=records.yaml proxy.config.diags.debug.enabled + + When no file name is given, write ``-c`` last, or use the ``--cold=`` form for the + explicit file. A bare ``-c`` followed by a record name is ambiguous, because the record + name is taken as the file name: + + .. code-block:: bash + + $ traffic_ctl config get proxy.config.diags.debug.enabled -c # default records.yaml + $ traffic_ctl config get -c proxy.config.diags.debug.enabled # wrong, reads a file + # named for the record + If the file exists and is empty a new document will be created. If a file does not exist, an attempt to create a new file will be done. This option(only for the config file changes) lets you use the prefix `proxy.config.` or `ts.` for variable names, either would work. diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index c15a552ff5a..b780656d82e 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -104,6 +104,29 @@ To add options to the parser or current command: This function call returns the new :class:`Option` instance. (0 is also number of arguments expected) +.. Note:: + + For options, the number of arguments may also be one of the following, which mirror the + ``nargs`` values of Python's ``argparse``: + + ================================ ======================================================= + Value Meaning + ================================ ======================================================= + ``AT_MOST_ONE_ARG_N`` Zero or one value (``argparse`` ``nargs='?'``) + ``MORE_THAN_ZERO_ARG_N`` Zero or more values (``argparse`` ``nargs='*'``) + ``MORE_THAN_ONE_ARG_N`` One or more values (``argparse`` ``nargs='+'``) + ================================ ======================================================= + + An option taking a variable number of values stops collecting when it reaches a token + naming another option of the same command, so options written afterwards keep their own + arguments. Use ``AT_MOST_ONE_ARG_N`` rather than ``MORE_THAN_ZERO_ARG_N`` for an option + whose value is optional, otherwise it also consumes the positional arguments of its + command. + + A ``--`` token stops option recognition for the values being collected, which is how a + value beginning with ``-`` is passed. Note this differs from the POSIX ``--``: it does + not end the value list nor force the remainder to be positional arguments. + We can also use the following chained way to add subcommand or option: .. code-block:: cpp diff --git a/include/tscore/ArgParser.h b/include/tscore/ArgParser.h index 57a2c0b6541..c0a428a38c0 100644 --- a/include/tscore/ArgParser.h +++ b/include/tscore/ArgParser.h @@ -34,10 +34,23 @@ constexpr unsigned MORE_THAN_ZERO_ARG_N = ~0; // more than one arguments constexpr unsigned MORE_THAN_ONE_ARG_N = ~0 - 1; +// zero or one argument +constexpr unsigned AT_MOST_ONE_ARG_N = ~0 - 2; // customizable indent for help message constexpr int INDENT_ONE = 32; constexpr int INDENT_TWO = 46; +/** Whether @a arg_num asks for a variable rather than a fixed number of values. + + Use this in preference to comparing against the sentinels, so that adding another + variable arity does not silently leave a sentinel being treated as a literal count. + */ +constexpr bool +is_variable_arg_num(unsigned arg_num) +{ + return arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N || arg_num == AT_MOST_ONE_ARG_N; +} + namespace ts { using AP_StrVec = std::vector; diff --git a/src/traffic_ctl/traffic_ctl.cc b/src/traffic_ctl/traffic_ctl.cc index 1bda6e8521d..c077b130934 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -118,7 +118,7 @@ main([[maybe_unused]] int argc, const char **argv) .add_example_usage("traffic_ctl config get [OPTIONS] RECORD [RECORD ...]") .add_option("--cold", "-c", "Save the value in a configuration file. This does not save the value in TS. Local file change only", - "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N) + "TS_RECORD_YAML", AT_MOST_ONE_ARG_N) .add_option("--records", "", "Emit output in YAML format") .add_option("--default", "", "Include default value"); config_command.add_command("match", "Get configuration matching a regular expression", "", MORE_THAN_ONE_ARG_N, Command_Execute) @@ -186,7 +186,7 @@ main([[maybe_unused]] int argc, const char **argv) config_command.add_command("set", "Set a configuration value", "", 2, Command_Execute) .add_option("--cold", "-c", "Save the value in a configuration file. This does not save the value in TS. Local file change only", - "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N) + "TS_RECORD_YAML", AT_MOST_ONE_ARG_N) .add_option("--update", "-u", "Update a configuration value. [only relevant if --cold set]") .add_option( "--type", "-t", diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 9c49f51c8e4..d295b36ea29 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -418,6 +418,8 @@ ArgParser::Command::output_option() const return {" [ ...]"}; } else if (num == MORE_THAN_ONE_ARG_N) { return {" ..."}; + } else if (num == AT_MOST_ONE_ARG_N) { + return {" []"}; } else { return " ... "; } @@ -541,6 +543,29 @@ ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string con ArgumentData data; ret.append(name, data); // handle the args + if (arg_num == AT_MOST_ONE_ARG_N) { + // Zero or one value. A value is taken only when the following token does not name + // another option of this command, which leaves this command's positional arguments + // in place. A "--" token makes whatever follows it a value rather than an option. + unsigned j{index + 1}; + bool takes_value{false}; + + if (j < args.size()) { + if (args[j] == "--") { + ++j; + takes_value = j < args.size(); + } else { + takes_value = !is_registered_option(args[j]); + } + } + if (takes_value) { + ret.append_arg(name, args[j]); + ++j; + } + args.erase(args.begin() + index, args.begin() + j); + index -= 1; + return ""; + } if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) { // 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 @@ -754,7 +779,7 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde // check for wrong number of arguments for --arg=... for (const auto &it : check_map) { unsigned num = _option_list.at(it.first).arg_num; - if (num != it.second && num < MORE_THAN_ONE_ARG_N) { + if (num != it.second && !is_variable_arg_num(num)) { help_message(std::to_string(_option_list.at(it.first).arg_num) + " arguments expected by " + it.first); } } diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index a3117e57e57..bba84323dca 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -288,3 +288,73 @@ TEST_CASE("Option value keeps embedded equal signs", "[parse]") REQUIRE(parsed.get("directive").size() == 1); REQUIRE(parsed.get("directive")[0] == "ip_allow.id=foo"); } + +TEST_CASE("An option taking at most one argument leaves the positional arguments alone", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl config get [--cold [FILE]] RECORD [RECORD ...]". + ts::ArgParser::Command &cmd = parser.add_command("get", "get values", "", MORE_THAN_ONE_ARG_N, nullptr); + cmd.add_option("--cold", "-c", "read from a file", "", AT_MOST_ONE_ARG_N); + cmd.add_option("--records", "", "yaml output"); + + // The option takes its single value and stops, so the command keeps its own arguments. + const char *argv1[] = {"test_prog", "get", "-c", "records.yaml", "proxy.config.x", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); + + // Several positional arguments are unaffected. + const char *argv2[] = {"test_prog", "get", "-c", "records.yaml", "proxy.config.x", "proxy.config.y", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 2); + REQUIRE(parsed.get("get")[1] == "proxy.config.y"); + + // Trailing placement keeps working. + const char *argv3[] = {"test_prog", "get", "proxy.config.x", "-c", "records.yaml", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); + + // The --option=value form is not mistaken for a fixed arity mismatch. + const char *argv4[] = {"test_prog", "get", "--cold=records.yaml", "proxy.config.x", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); +} + +TEST_CASE("An option taking at most one argument accepts no value at all", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("get", "get values", "", MORE_THAN_ONE_ARG_N, nullptr); + cmd.add_option("--cold", "-c", "read from a file", "", AT_MOST_ONE_ARG_N); + cmd.add_option("--records", "", "yaml output"); + + // Called with no value, so the caller falls back to its own default. + const char *argv1[] = {"test_prog", "get", "proxy.config.x", "-c", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("cold") == true); + REQUIRE(parsed.get("cold").size() == 0); + REQUIRE(parsed.get("cold").value().empty()); + REQUIRE(parsed.get("get").size() == 1); + + // A following option is never taken as the value. + const char *argv2[] = {"test_prog", "get", "-c", "--records", "proxy.config.x", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("cold").size() == 0); + REQUIRE(parsed.get("records") == true); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); + + // After "--" even a token shaped like an option becomes the value. + const char *argv3[] = {"test_prog", "get", "-c", "--", "-weird-name.yaml", "proxy.config.x", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("cold").value() == "-weird-name.yaml"); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); +} diff --git a/tests/gold_tests/records/traffic_ctl_cold_config.test.py b/tests/gold_tests/records/traffic_ctl_cold_config.test.py index c27925baf2b..b9034b3c511 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -92,3 +92,43 @@ tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Env = ts.Env tr.Disk.File(file).Content = 'gold/records.yaml.cold_test5.gold' + +# --cold takes at most one file name, so it does not consume the record names that follow +# it. Before that was the case it had to be written after them, which the runs above do. +records_file = os.path.join(ts.Variables.CONFIGDIR, "records.yaml") + +# 6 +tr = Test.AddTestRun("Get a value with the file name given before the record") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The record must still be parsed as a record') + +# 7 +tr = Test.AddTestRun("Get several values with the file name given before them") +tr.Processes.Default.Command = ( + f'traffic_ctl config get -c {records_file} ' + 'proxy.config.diags.debug.tags proxy.config.cache.limits.http.max_alts') +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The first record must be parsed as a record') +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.cache.limits.http.max_alts: 1', 'The second record must be parsed as a record') + +# 8 +tr = Test.AddTestRun("Get a value using the --cold=FILE form") +tr.Processes.Default.Command = f'traffic_ctl config get --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The record must still be parsed as a record') + +# 9 +file = os.path.join(ts.Variables.CONFIGDIR, "new_records3.yaml") +tr = Test.AddTestRun("Set a value with the file name given before the record and the value") +tr.Processes.Default.Command = f'traffic_ctl config set -c {file} proxy.config.cache.limits.http.max_alts 3' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Disk.File(file).Content = 'gold/records.yaml.cold_test5.gold' From 087d7b6506db01519275dfbad45f134292453963 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 26 Aug 2026 09:41:41 +0200 Subject: [PATCH 4/9] Correct the variable-arg comment in handle_args 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. --- src/tscore/ArgParser.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index d295b36ea29..495edf8ac0c 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -567,10 +567,11 @@ ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string con return ""; } if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) { - // 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. + // Variable number of arguments. Stop collecting at a token that names another option of this + // command, so options written afterwards keep their own values. Every other token is taken as + // a value, including a positional argument of the command, which is why an option whose value + // is optional wants AT_MOST_ONE_ARG_N rather than MORE_THAN_ZERO_ARG_N. A "--" token ends + // option recognition, which is how a value that starts with '-' can be passed. unsigned j{index + 1}; unsigned collected{0}; bool recognize_options{true}; From 5367a988fdac0f2a1195b865c6cca9cdc15e426d Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 26 Aug 2026 14:14:38 +0200 Subject: [PATCH 5/9] Stop fixed-arity options consuming later options 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 '-'. --- .../internal-libraries/ArgParser.en.rst | 4 +++ src/tscore/ArgParser.cc | 25 ++++++++++++---- src/tscore/unit_tests/test_ArgParser.cc | 30 +++++++++++++++++++ .../traffic_ctl_server_debug.test.py | 18 +++++++++++ 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index b780656d82e..0171c4a6908 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -123,6 +123,10 @@ This function call returns the new :class:`Option` instance. (0 is also number o whose value is optional, otherwise it also consumes the positional arguments of its command. + A token naming another option is not a value for a fixed number of arguments either. An + option written where a value is expected leaves the value missing, which is reported as a + usage error rather than the option being consumed and applied as the value. + A ``--`` token stops option recognition for the values being collected, which is how a value beginning with ``-`` is passed. Note this differs from the POSIX ``--``: it does not end the value list nor force the remainder to be positional arguments. diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 495edf8ac0c..b2c0b29b7b3 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -596,15 +596,30 @@ ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string con index -= 1; return ""; } - // finite number of argument handling - for (unsigned j = 0; j < arg_num; j++) { - if (args.size() < index + j + 2 || args[index + j + 1].empty()) { + // Fixed number of arguments. A token naming another option of this command is not a value, so + // the missing value is reported rather than the following option being consumed as one. A "--" + // token ends option recognition, which is how a value that starts with '-' is passed. + unsigned j{index + 1}; + bool recognize_options{true}; + + for (unsigned collected{0}; collected < arg_num; ++j) { + if (j >= args.size() || args[j].empty()) { return std::to_string(arg_num) + " argument(s) expected by " + name; } - ret.append_arg(name, args[index + j + 1]); + if (recognize_options) { + if (args[j] == "--") { + recognize_options = false; + continue; + } + if (is_registered_option(args[j])) { + return std::to_string(arg_num) + " argument(s) expected by " + name; + } + } + ret.append_arg(name, args[j]); + ++collected; } // erase the used arguments and append the data to the return structure - args.erase(args.begin() + index, args.begin() + index + arg_num + 1); + args.erase(args.begin() + index, args.begin() + j); index -= 1; return ""; } diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index bba84323dca..dc10cc4e86b 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -358,3 +358,33 @@ TEST_CASE("An option taking at most one argument accepts no value at all", "[par REQUIRE(parsed.get("get").size() == 1); REQUIRE(parsed.get("get")[0] == "proxy.config.x"); } + +TEST_CASE("An option taking a fixed number of arguments can be given a value shaped like an option", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl server debug enable [--tags TAGS] [--append]". + ts::ArgParser::Command &cmd = parser.add_command("enable", "enable debug"); + cmd.add_option("--tags", "-t", "debug tags", "", 1); + cmd.add_option("--append", "-a", "append to the existing tags"); + + // A value that starts with '-' is passed after "--", which is otherwise taken as naming an + // option and reported as a missing value. + const char *argv1[] = {"test_prog", "enable", "-t", "--", "-a", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("tags").value() == "-a"); + REQUIRE(parsed.get("append") == false); + + // The --option=value form needs no escape. + const char *argv2[] = {"test_prog", "enable", "--tags=-a", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("tags").value() == "-a"); + REQUIRE(parsed.get("append") == false); + + // An option written after the value keeps its own meaning. + const char *argv3[] = {"test_prog", "enable", "-t", "http", "-a", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("tags").value() == "http"); + REQUIRE(parsed.get("append") == true); +} diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py index f899f980708..30b781732b2 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py @@ -78,3 +78,21 @@ tr.Processes.Default.Streams.All = Testers.ContainsExpression( "Option \'--append\' requires \'--tags\' to be specified", "Should show error that --append requires --tags") tr.StillRunningAfter = traffic_ctl._ts + +# Test 15: An option written where the tags are expected leaves them missing, rather than being +# applied as the tags themselves. +tr = Test.AddTestRun("test --tags followed by another option") +tr.Processes.Default.Env = traffic_ctl._ts.Env +tr.Processes.Default.Command = "traffic_ctl server debug enable --tags --append" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "1 argument\\(s\\) expected by tags", "Should report the tags as missing") +tr.StillRunningAfter = traffic_ctl._ts + +# Test 16: Tags that are shaped like an option are passed after "--". +tr = Test.AddTestRun("test tags shaped like an option") +tr.Processes.Default.Env = traffic_ctl._ts.Env +tr.Processes.Default.Command = "traffic_ctl server debug enable --tags -- -a" +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = Testers.ContainsExpression('tags »"-a"«', "The value after -- must be taken as the tags") +tr.StillRunningAfter = traffic_ctl._ts From c09f1aa0f81860ce21883cd8855e3888965c9163 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 11:18:15 +0200 Subject: [PATCH 6/9] Reject a repeated at-most-one-argument option 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. --- src/tscore/ArgParser.cc | 12 +++++++++--- .../records/traffic_ctl_cold_config.test.py | 8 ++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index b2c0b29b7b3..a572b72f488 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -794,9 +794,15 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde } // check for wrong number of arguments for --arg=... for (const auto &it : check_map) { - unsigned num = _option_list.at(it.first).arg_num; - if (num != it.second && !is_variable_arg_num(num)) { - help_message(std::to_string(_option_list.at(it.first).arg_num) + " arguments expected by " + it.first); + unsigned const num = _option_list.at(it.first).arg_num; + if (num == AT_MOST_ONE_ARG_N) { + // At most one, so a repeated option is as wrong as a repeated fixed arity one, which + // is_variable_arg_num() would otherwise wave through. + if (it.second > 1) { + help_message("at most one argument expected by " + it.first); + } + } else if (num != it.second && !is_variable_arg_num(num)) { + help_message(std::to_string(num) + " arguments expected by " + it.first); } } } diff --git a/tests/gold_tests/records/traffic_ctl_cold_config.test.py b/tests/gold_tests/records/traffic_ctl_cold_config.test.py index b9034b3c511..aed15c53878 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -132,3 +132,11 @@ tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Env = ts.Env tr.Disk.File(file).Content = 'gold/records.yaml.cold_test5.gold' + +# 10 +tr = Test.AddTestRun("--cold takes at most one file name, so repeating it is an error") +tr.Processes.Default.Command = f'traffic_ctl config get --cold={records_file} --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'A repeated --cold must be reported rather than the last one winning') From c94ce9145db3cffbb16b2a9b01b29dad65ee770b Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 11:18:15 +0200 Subject: [PATCH 7/9] Correct the bare -c example in the traffic_ctl docs 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. --- doc/appendices/command-line/traffic_ctl.en.rst | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index f0f2aeeea6c..e2169c1e1e2 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -579,14 +579,17 @@ Display the current value of a configuration record. $ traffic_ctl config get --cold=records.yaml proxy.config.diags.debug.enabled When no file name is given, write ``-c`` last, or use the ``--cold=`` form for the - explicit file. A bare ``-c`` followed by a record name is ambiguous, because the record - name is taken as the file name: + explicit file. A bare ``-c`` followed by a record name takes the record as the file name, + which leaves the command with no records of its own and is reported as a usage error: .. code-block:: bash $ traffic_ctl config get proxy.config.diags.debug.enabled -c # default records.yaml - $ traffic_ctl config get -c proxy.config.diags.debug.enabled # wrong, reads a file - # named for the record + $ traffic_ctl config get -c proxy.config.diags.debug.enabled + Error: at least one argument expected by get + + ``-c`` is also given at most once, so repeating it is a usage error rather than the last + file name silently winning. If the file exists and is empty a new document will be created. If a file does not exist, an attempt to create a new file will be done. From 73d26466c53728d164ae853d505540e4b0d08690 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 13:18:45 +0200 Subject: [PATCH 8/9] Count both --cold spellings against the at-most-one limit 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. --- src/tscore/ArgParser.cc | 5 +++++ .../records/traffic_ctl_cold_config.test.py | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index a572b72f488..090c48ae0e6 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -779,6 +779,11 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde } else { cur_option = _option_list.at(short_it->second); } + // Counted for the same repetition check as the --option=value form, so that an option + // taking at most one value cannot be given more by mixing the two spellings. + if (cur_option.arg_num == AT_MOST_ONE_ARG_N) { + check_map[cur_option.long_option] += 1; + } // handle the arguments std::string err = handle_args(ret, args, cur_option.key, cur_option.arg_num, i); if (!err.empty()) { diff --git a/tests/gold_tests/records/traffic_ctl_cold_config.test.py b/tests/gold_tests/records/traffic_ctl_cold_config.test.py index aed15c53878..219a9a7b766 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -140,3 +140,19 @@ tr.Processes.Default.Env = ts.Env tr.Processes.Default.Streams.All = Testers.ContainsExpression( 'at most one argument expected by --cold', 'A repeated --cold must be reported rather than the last one winning') + +# 11 +tr = Test.AddTestRun("A repeated --cold is an error in the space-separated form too") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} -c {records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'A repeated -c must be reported rather than the last one winning') + +# 12 +tr = Test.AddTestRun("Mixing the two --cold spellings cannot smuggle in a second file name") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'The two forms must be counted together') From 68e57089ac46497267b195bf136e8951b9383908 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 28 Aug 2026 13:18:46 +0200 Subject: [PATCH 9/9] Say what "--" does to the options that follow it 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. --- doc/appendices/command-line/traffic_ctl.en.rst | 10 ++++++++-- .../internal-libraries/ArgParser.en.rst | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index e2169c1e1e2..fbe180995b0 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -465,8 +465,14 @@ Display the current value of a configuration record. $ traffic_ctl config reload -D myconfig.id=foo --monitor $ traffic_ctl config reload -D myconfig.id=foo -d 'myconfig: {rules: [a]}' - To pass a directive value that begins with ``-``, place ``--`` before it; every - token after ``--`` is taken as a value rather than an option. + To pass a directive value that begins with ``-``, place ``--`` before it. Option + recognition then stays off for the rest of the line, so every remaining token becomes + a directive value and any option written afterwards is swallowed. Use + ``--directive=-value`` instead when options still have to follow: + + .. code-block:: bash + + $ traffic_ctl config reload --directive=-weird.id=foo --monitor .. note:: diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index 0171c4a6908..c47801a98b5 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -131,6 +131,11 @@ This function call returns the new :class:`Option` instance. (0 is also number o value beginning with ``-`` is passed. Note this differs from the POSIX ``--``: it does not end the value list nor force the remainder to be positional arguments. + Option recognition stays off for the rest of that collection, so for a variable number of + values every remaining token becomes a value and no later option is recognized. Use the + ``--option=value`` form instead when options still have to follow a value that begins with + ``-``. + We can also use the following chained way to add subcommand or option: .. code-block:: cpp