redis-server command line arguments support take one bulk string with spaces for MULTI_ARG configs parsing. And allow options value to use the -- prefix by enjoy-binbin · Pull Request #10660 · redis/redis

enjoy-binbin · 2022-04-28T07:16:55Z

Take one bulk string with spaces for MULTI_ARG configs parsing

Currently redis-server looks for arguments that start with --,
and anything in between them is considered arguments for the config.
like: src/redis-server --shutdown-on-sigint nosave force now --port 6380

MULTI_ARG configs behave differently for CONFIG command, vs the command
line argument for redis-server.
i.e. CONFIG command takes one bulk string with spaces in it, while the
command line takes an argv array with multiple values.

In this PR, in config.c, if argc > 1 we can take them as is,
and if the config is a MULTI_ARG and argc == 1, we will split it by spaces.

So both of these will be the same:

redis-server --shutdown-on-sigint nosave force now --shutdown-on-sigterm nosave force
redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm nosave force
redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm "nosave force"

Allow options value to use the `--` prefix

Currently it decides to switch to the next config, as soon as it sees --,
even if there was not a single value provided yet to the last config,
this makes it impossible to define a config value that has -- prefix in it.

For instance, if we want to set the logfile to --my--log--file,
like redis-server --logfile --my--log--file --loglevel verbose,
current code will handle that incorrectly.

In this PR, now we allow a config value that has -- prefix in it.
But note that something like redis-server --some-config --config-value1 --config-value2 --loglevel debug
would not work, because if you want to pass a value to a config starting with --, it can only be a single value.
like: redis-server --some-config "--config-value1 --config-value2" --loglevel debug

An example (using -- prefix config value):

redis-server --logfile --my--log--file --loglevel verbose
redis-cli config get logfile loglevel
1) "loglevel"
2) "verbose"
3) "logfile"
4) "--my--log--file"

This is mentioned in #10594 (comment)

Potentially breaking change

redis-server --save --loglevel verbose used to work the same as redis-server --save "" --loglevel verbose
now, it'll error!

Note that this breaking change was undo in #10866 (7.0.3)

… spaces for MULTI_ARG configs parsing Currently redis-server looks for arguments that start with `--`, and anything in between them is considered arguments for the config. like: `src/redis-server --shutdown-on-sigint nosave force now --port 6380` MULTI_ARG configs behave differently for CONFIG command, vs the command line argument for redis-server. i.e. CONFIG command takes one bulk string with spaces in it, while the command line takes an argv array with multiple values. In this PR, in config.c, if `argc > 1` we can take them as is, and if the config is a `MULTI_ARG` and `argc == 1`, we will split it by spaces. So both of these will be the same: ``` redis-server --shutdown-on-sigint nosave force now --port 6380 redis-server --shutdown-on-sigint "nosave force now" --port 6380 ``` And for MULTI_ARG, now we will check that it has at least one arg, we can return `wrong number of arguments` before `config->interface.set`. ``` old: src/redis-server --shutdown-on-sigint >>> 'shutdown-on-sigint' argument(s) must be one of the following: default, save, nosave, now, force new: src/redis-server --shutdown-on-sigint >>> 'shutdown-on-sigint' wrong number of arguments ``` This is mentioned in redis#10594 (comment) Co-authored-by: Oran Agra <[email protected]>

src/server.h

oranagra · 2022-04-28T07:45:51Z

@enjoy-binbin thank you.
i didn't review the code yet, but i wanna mention some feedback from the description.
I'm not yet sure if the empty arg thing is necessary, i think maybe each of the multi-arg configs can do it's own arity checks.

what i did want to handle is the config matching in server.c.
currently it decides to switch to the next config, as soon as it sees --, even if there was not a single value provided yet to the last config, this makes it impossible to define a config value that has -- prefix in it.
for instance, if i want to set the logfile to --my--log--file.
./redis-server --logfile --my--log--file --loglevel verbose
current code will handle that incorrectly, and i think all that's needed to do in order to fix, is to avoid matching the first value of a config with --, and just assume it must be a value.

sundb · 2022-04-28T07:50:55Z

I don't like ALLOW_EMPTY_ARG_CONFIG, it makes me feel that this config is allowing args to be empty.

src/config.c

tests/unit/introspection.tcl

enjoy-binbin · 2022-04-28T08:58:45Z

@oranagra I misunderstood what you want before (like i did try touch --my--log--file and ended out touch: unrecognized option), so i support using -- is a bad case.
And thought that you want is do a arity checks before the config set

Now with the new code, the result is:

src/redis-server --logfile --my--log--file --loglevel verbose --shutdown-on-sigint nosave force now --shutdown-on-sigterm "nosave force"

127.0.0.1:6379> config get logfile loglevel shutdown-on-sigint shutdown-on-sigterm
1) "loglevel"
2) "verbose"
3) "shutdown-on-sigterm"
4) "nosave force"
5) "shutdown-on-sigint"
6) "nosave now force"
7) "logfile"
8) "--my--log--file"

src/server.c

oranagra

LGTM.
@yoav-steinberg @yossigo @madolson PTAL

src/config.c

src/server.c

Co-authored-by: yoav-steinberg <[email protected]>

…xample

Allow option value to use `--` prefix will be a breaking change, will break: src/redis-server --save --loglevel verbose It used to work the same as: src/redis-server --save "" --loglevel verbose revert it

src/server.c

This reverts commit 19c9771.

oranagra

ohh, took down some of these notes hours ago, now i realize i forgot to post them.

tests/unit/introspection.tcl

…mmand_line_args

save "", sdssplitargs(argv[1], &new_argc) the new_argc will become 0, and will break save ""

src/config.c

Co-authored-by: Oran Agra <[email protected]>

oranagra · 2022-05-09T14:05:04Z

@redis/core-team please approve or comment (note the breaking change).

…ue in the same arg (#10866) This commit has two topics. ## Passing config name and value in the same arg In #10660 (Redis 7.0.1), when we supported the config values that can start with `--` prefix (one of the two topics of that PR), we broke another pattern: `redis-server redis.config "name value"`, passing both config name and it's value in the same arg, see #10865 This wasn't a intended change (i.e we didn't realize this pattern used to work). Although this is a wrong usage, we still like to fix it. Now we support something like: ``` src/redis-server redis.conf "--maxmemory '700mb'" "--maxmemory-policy volatile-lru" --proc-title-template --my--title--template --loglevel verbose ``` ## Changes around --save Also in this PR, we undo the breaking change we made in #10660 on purpose. 1. `redis-server redis.conf --save --loglevel verbose` (missing `save` argument before anotehr argument). In 7.0.1, it was throwing an wrong arg error. Now it will work and reset the save, similar to how it used to be in 7.0.0 and 6.2.x. 3. `redis-server redis.conf --loglevel verbose --save` (missing `save` argument as last argument). In 6.2, it did not reset the save, which was a bug (inconsistent with the previous bullet). Now we will make it work and reset the save as well (a bug fix).

There is a issue with --sentinel: ``` [root]# src/redis-server sentinel.conf --sentinel --loglevel verbose *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 352 >>> 'sentinel "--loglevel" "verbose"' Unrecognized sentinel configuration statement ``` This is because in redis#10660 (Redis 7.0.1), `--` prefix change break it. In this PR, we will handle `--sentinel` the same as we did for `--save` in redis#10866. i.e. it's a pseudo config option with no value. Fixes redis#11586. Co-authored-by: Oran Agra <[email protected]>

There is a issue with --sentinel: ``` [root]# src/redis-server sentinel.conf --sentinel --loglevel verbose *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 352 >>> 'sentinel "--loglevel" "verbose"' Unrecognized sentinel configuration statement ``` This is because in #10660 (Redis 7.0.1), `--` prefix change break it. In this PR, we will handle `--sentinel` the same as we did for `--save` in #10866. i.e. it's a pseudo config option with no value.

There is a issue with --sentinel: ``` [root]# src/redis-server sentinel.conf --sentinel --loglevel verbose *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 352 >>> 'sentinel "--loglevel" "verbose"' Unrecognized sentinel configuration statement ``` This is because in #10660 (Redis 7.0.1), `--` prefix change break it. In this PR, we will handle `--sentinel` the same as we did for `--save` in #10866. i.e. it's a pseudo config option with no value. (cherry picked from commit 8f13ac1)

There is a issue with --sentinel: ``` [root]# src/redis-server sentinel.conf --sentinel --loglevel verbose *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 352 >>> 'sentinel "--loglevel" "verbose"' Unrecognized sentinel configuration statement ``` This is because in redis#10660 (Redis 7.0.1), `--` prefix change break it. In this PR, we will handle `--sentinel` the same as we did for `--save` in redis#10866. i.e. it's a pseudo config option with no value.

… spaces for MULTI_ARG configs parsing. And allow options value to use the -- prefix (redis#10660) ## Take one bulk string with spaces for MULTI_ARG configs parsing Currently redis-server looks for arguments that start with `--`, and anything in between them is considered arguments for the config. like: `src/redis-server --shutdown-on-sigint nosave force now --port 6380` MULTI_ARG configs behave differently for CONFIG command, vs the command line argument for redis-server. i.e. CONFIG command takes one bulk string with spaces in it, while the command line takes an argv array with multiple values. In this PR, in config.c, if `argc > 1` we can take them as is, and if the config is a `MULTI_ARG` and `argc == 1`, we will split it by spaces. So both of these will be the same: ``` redis-server --shutdown-on-sigint nosave force now --shutdown-on-sigterm nosave force redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm nosave force redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm "nosave force" ``` ## Allow options value to use the `--` prefix Currently it decides to switch to the next config, as soon as it sees `--`, even if there was not a single value provided yet to the last config, this makes it impossible to define a config value that has `--` prefix in it. For instance, if we want to set the logfile to `--my--log--file`, like `redis-server --logfile --my--log--file --loglevel verbose`, current code will handle that incorrectly. In this PR, now we allow a config value that has `--` prefix in it. **But note that** something like `redis-server --some-config --config-value1 --config-value2 --loglevel debug` would not work, because if you want to pass a value to a config starting with `--`, it can only be a single value. like: `redis-server --some-config "--config-value1 --config-value2" --loglevel debug` An example (using `--` prefix config value): ``` redis-server --logfile --my--log--file --loglevel verbose redis-cli config get logfile loglevel 1) "loglevel" 2) "verbose" 3) "logfile" 4) "--my--log--file" ``` ### Potentially breaking change `redis-server --save --loglevel verbose` used to work the same as `redis-server --save "" --loglevel verbose` now, it'll error!

…ue in the same arg (redis#10866) This commit has two topics. ## Passing config name and value in the same arg In redis#10660 (Redis 7.0.1), when we supported the config values that can start with `--` prefix (one of the two topics of that PR), we broke another pattern: `redis-server redis.config "name value"`, passing both config name and it's value in the same arg, see redis#10865 This wasn't a intended change (i.e we didn't realize this pattern used to work). Although this is a wrong usage, we still like to fix it. Now we support something like: ``` src/redis-server redis.conf "--maxmemory '700mb'" "--maxmemory-policy volatile-lru" --proc-title-template --my--title--template --loglevel verbose ``` ## Changes around --save Also in this PR, we undo the breaking change we made in redis#10660 on purpose. 1. `redis-server redis.conf --save --loglevel verbose` (missing `save` argument before anotehr argument). In 7.0.1, it was throwing an wrong arg error. Now it will work and reset the save, similar to how it used to be in 7.0.0 and 6.2.x. 3. `redis-server redis.conf --loglevel verbose --save` (missing `save` argument as last argument). In 6.2, it did not reset the save, which was a bug (inconsistent with the previous bullet). Now we will make it work and reset the save as well (a bug fix).

There is a issue with --sentinel: ``` [root]# src/redis-server sentinel.conf --sentinel --loglevel verbose *** FATAL CONFIG FILE ERROR (Redis 255.255.255) *** Reading the configuration file, at line 352 >>> 'sentinel "--loglevel" "verbose"' Unrecognized sentinel configuration statement ``` This is because in redis#10660 (Redis 7.0.1), `--` prefix change break it. In this PR, we will handle `--sentinel` the same as we did for `--save` in redis#10866. i.e. it's a pseudo config option with no value.

sundb reviewed Apr 28, 2022

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

oranagra added the 7.0-must-have label Apr 28, 2022

fix copy-paste bug, ALLOW_EMPTY_ARG_CONFIG

3f32559

oranagra reviewed Apr 28, 2022

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

src/config.c Outdated Show resolved Hide resolved

tests/unit/introspection.tcl Outdated Show resolved Hide resolved

Remove ALLOW_EMPTY_ARG_CONFIG and avoid matching --

458dc51

enjoy-binbin commented Apr 28, 2022

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

enjoy-binbin and others added 4 commits April 28, 2022 17:42

add last_config_have_value condition

01c8e91

add tests and cleanups

00ce563

small cleanup (i hope)

802f037

typo

0b98adc

oranagra approved these changes Apr 28, 2022

View reviewed changes

yoav-steinberg reviewed Apr 28, 2022

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

enjoy-binbin and others added 2 commits April 29, 2022 00:26

apply yoav's suggestions: refactor && handled_last_config_arg

fa88687

Co-authored-by: yoav-steinberg <[email protected]>

remove duplicate test && redis-server --replicaof --127.0.0.1 --abc e…

9b868cd

…xample

yoav-steinberg approved these changes May 1, 2022

View reviewed changes

yossigo approved these changes May 1, 2022

View reviewed changes

madolson approved these changes May 2, 2022

View reviewed changes

revert "allow options value to use the -- prefix"

19c9771

Allow option value to use `--` prefix will be a breaking change, will break: src/redis-server --save --loglevel verbose It used to work the same as: src/redis-server --save "" --loglevel verbose revert it

yoav-steinberg reviewed May 3, 2022

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

enjoy-binbin added 2 commits May 8, 2022 17:34

Revert "revert "allow options value to use the -- prefix""

817fbe4

This reverts commit 19c9771.

add save empty input test case

f8e76e8

oranagra reviewed May 8, 2022

View reviewed changes

tests/unit/introspection.tcl Outdated Show resolved Hide resolved

tests/unit/introspection.tcl Outdated Show resolved Hide resolved

enjoy-binbin added 3 commits May 9, 2022 18:49

Merge remote-tracking branch 'upstream/unstable' into redis_server_co…

6d22026

…mmand_line_args

handle save "" in MULTI_ARG_CONFIG split logic

e677b01

save "", sdssplitargs(argv[1], &new_argc) the new_argc will become 0, and will break save ""

simplify the tests

e73d7fe

oranagra approved these changes May 9, 2022

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

Update src/config.c

603800f

Co-authored-by: Oran Agra <[email protected]>

oranagra added breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes labels May 9, 2022

yossigo approved these changes May 11, 2022

View reviewed changes

oranagra merged commit bfbb15f into redis:unstable May 11, 2022

enjoy-binbin deleted the redis_server_command_line_args branch May 11, 2022 08:41

oranagra mentioned this pull request Jun 8, 2022

Release 7.0.1 #10829

Merged

This was referenced Jun 15, 2022

[CRASH] 7.0.2 crash loop with parameter provide from k8s deployment #10865

Closed

redis-server command line arguments allow passing config name and value in the same arg #10866

Merged

enjoy-binbin mentioned this pull request Dec 6, 2022

[BUG] sentinel option gone #11586

Closed

enjoy-binbin mentioned this pull request Dec 6, 2022

Fix command line startup --sentinel problem #11591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redis-server command line arguments support take one bulk string with spaces for MULTI_ARG configs parsing. And allow options value to use the -- prefix#10660

redis-server command line arguments support take one bulk string with spaces for MULTI_ARG configs parsing. And allow options value to use the -- prefix#10660
oranagra merged 16 commits intoredis:unstablefrom
enjoy-binbin:redis_server_command_line_args

enjoy-binbin commented Apr 28, 2022 •

edited

Loading

Uh oh!

Uh oh!

oranagra commented Apr 28, 2022

Uh oh!

sundb commented Apr 28, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enjoy-binbin commented Apr 28, 2022 •

edited

Loading

Uh oh!

Uh oh!

oranagra left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

enjoy-binbin commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Take one bulk string with spaces for MULTI_ARG configs parsing

Allow options value to use the -- prefix

Potentially breaking change

Uh oh!

Uh oh!

oranagra commented Apr 28, 2022

Uh oh!

sundb commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enjoy-binbin commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

enjoy-binbin commented Apr 28, 2022 •

edited

Loading

Allow options value to use the `--` prefix

sundb commented Apr 28, 2022 •

edited

Loading

enjoy-binbin commented Apr 28, 2022 •

edited

Loading