brbzull0 opened a new issue, #13569:
URL: https://github.com/apache/trafficserver/issues/13569
## Summary
`ArgParser` options declared with `MORE_THAN_ZERO_ARG_N` /
`MORE_THAN_ONE_ARG_N`
consume **every** remaining token on the command line. That includes tokens
that
are registered options of the same command, and the command's own positional
arguments.
The practical effect is that a variable-argument option must be written
last, and
the argument order advertised by `--help` for several `traffic_ctl` commands
does
not actually work.
## Reproducers
All against `master`.
The documented order for `config get` fails:
```console
$ traffic_ctl config get -c proxy.config.diags.debug.enabled
Error: at least one argument expected by get
```
`--cold, -c` is declared `MORE_THAN_ZERO_ARG_N`, so it consumes the record
name
and `get` is left with no positional argument — even though
`traffic_ctl config get --help` advertises
`traffic_ctl config get [OPTIONS] RECORD [RECORD ...]`.
Same for `config set`:
```console
$ traffic_ctl config set -c proxy.config.diags.debug.enabled 1
Error: 2 argument(s) expected by set
```
Both work only when the variable-argument option is written last, which is
what
the documentation examples already do:
```console
$ traffic_ctl config get proxy.config.diags.debug.enabled -c
$ traffic_ctl config set proxy.config.diags.debug.enabled 1 -c records.yaml
```
An option following a variable-argument option at the same command level is
also
swallowed:
```console
$ traffic_ctl config reload -D ip_allow.id=foo -m
Error: '-m' looks like a flag, not a directive. Place -D as the last option
on the command line.
```
Note that *global* options are not affected, because `Command::parse()` runs
the
top-level `append_option_data()` pass before recursing into subcommands, so
they
have already been removed from the token vector. For example
`traffic_ctl config reload -D ip_allow.id=foo -f rpc` parses correctly. Only
options registered on the same command as the variable-argument option are
lost.
## Root cause
`handle_args()` in `src/tscore/ArgParser.cc`:
```cpp
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++) {
ret.append_arg(name, args[j]);
}
args.erase(args.begin() + index, args.end());
return "";
}
```
There is no check for option-like tokens, and the erase truncates all the
way to
`args.end()`. That produces two distinct failures:
1. Options appearing after the variable-argument option are never matched,
because `append_option_data()` is still iterating over a vector that has
just
been truncated.
2. The command's own positional arguments are discarded, because
`Command::parse()` calls `append_option_data()` (options) *before*
`handle_args()` (the command's positionals).
`ArgParser` also has no support for `--` as an end-of-options marker, so
there is
currently no way to pass a value that legitimately begins with `-`.
For comparison, Python's `argparse` stops consuming `nargs='*'` / `nargs='+'`
values at the next token that looks like a registered option.
## Affected declarations
Variable-argument **options** (the ones that can lose following tokens):
- `src/traffic_ctl/traffic_ctl.cc` — `--cold, -c` on `config get` and
`config set`, `--data, -d` and `--directive, -D` on `config reload`,
`--params, -p` on `rpc invoke`
Variable-argument **commands** (`config describe`, `config get`, `config
match`,
`host up/down`, `metric get/describe/match`, `plugin msg`, `storage offline`,
`rpc file`, ...) are not directly affected, since options are stripped
before the
command's positionals are collected. They are only affected indirectly, as
the
victims of case 2 above.
## Suggested direction
- Stop collecting values at the next token that matches a registered option
of
the current command, and erase only the consumed range rather than
truncating
to `args.end()`.
- Add support for `--` as an end-of-options marker so values that
legitimately
begin with `-` remain expressible.
- Any change here needs an audit for consumers that pass values starting with
`-`, to avoid a behaviour regression.
## Current workaround
PR #13110 added a `-D`-specific guard in `traffic_ctl` that detects tokens
starting with `-` and emits an actionable error, plus a note in
`doc/appendices/command-line/traffic_ctl.en.rst` stating that `-D` must be
the
last option. Both should be removed once `ArgParser` handles this correctly.
## Acceptance criteria
- [ ] `traffic_ctl config get -c RECORD` and `traffic_ctl config set -c
RECORD VALUE` work.
- [ ] `traffic_ctl config reload -D a=1 b=2 -m` parses correctly.
- [ ] Unit test: variable-argument option followed by another registered
option.
- [ ] Unit test: variable-argument option followed by the command's
positional arguments.
- [ ] Unit test: `--` ends option parsing for a variable-argument option.
- [ ] The `traffic_ctl` `-D` guard and the "must be last option"
documentation note are removed.
- [ ] No regressions for existing variable-argument consumers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
