gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

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)

Browse Source 
## 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!
This commit is contained in:
Binbin 2022-05-11 16:33:35 +08:00 committed by GitHub
parent 783b210db4
commit bfbb15f75d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 88 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
src/config.c
View File

@ -487,9 +487,23 @@ void loadServerConfigFromString(char *config) {
err = "wrong number of arguments";
goto loaderr;
}
/* Set config using all arguments that follows */
if (!config->interface.set(config, &argv[1], argc-1, &err)) {
goto loaderr;
if ((config->flags & MULTI_ARG_CONFIG) && argc == 2 && sdslen(argv[1])) {
/* For MULTI_ARG_CONFIGs, if we only have one argument, try to split it by spaces.
* Only if the argument is not empty, otherwise something like --save "" will fail.
* So that we can support something like --config "arg1 arg2 arg3". */
sds *new_argv;
int new_argc;
new_argv = sdssplitargs(argv[1], &new_argc);
if (!config->interface.set(config, new_argv, new_argc, &err)) {
goto loaderr;
}
sdsfreesplitres(new_argv, new_argc);
} else {
/* Set config using all arguments that follows */
if (!config->interface.set(config, &argv[1], argc-1, &err)) {
goto loaderr;
}
}
sdsfreesplitres(argv,argc);

9
src/server.c
View File

@ -6959,6 +6959,7 @@ int main(int argc, char **argv) {
server.exec_argv[1] = zstrdup(server.configfile);
j = 2; // Skip this arg when parsing options
}
int handled_last_config_arg = 1;
while(j < argc) {
/* Either first or last argument - Should we read config from stdin? */
if (argv[j][0] == '-' && argv[j][1] == '\0' && (j == 1 || j == argc-1)) {
@ -6967,16 +6968,20 @@ int main(int argc, char **argv) {
/* All the other options are parsed and conceptually appended to the
* configuration file. For instance --port 6380 will generate the
* string "port 6380\n" to be parsed after the actual config file
* and stdin input are parsed (if they exist). */
else if (argv[j][0] == '-' && argv[j][1] == '-') {
* and stdin input are parsed (if they exist).
* Only consider that if the last config has at least one argument. */
else if (handled_last_config_arg && argv[j][0] == '-' && argv[j][1] == '-') {
/* Option name */
if (sdslen(options)) options = sdscat(options,"\n");
/* argv[j]+2 for removing the preceding `--` */
options = sdscat(options,argv[j]+2);
options = sdscat(options," ");
handled_last_config_arg = 0;
} else {
/* Option argument */
options = sdscatrepr(options,argv[j],strlen(argv[j]));
options = sdscat(options," ");
handled_last_config_arg = 1;
}
j++;
}

64
tests/unit/introspection.tcl
View File

@ -464,6 +464,70 @@ start_server {tags {"introspection"}} {
assert {[dict exists $res bind]}
}
test {redis-server command line arguments - error cases} {
catch {exec src/redis-server --port} err
assert_match {*'port'*wrong number of arguments*} $err
catch {exec src/redis-server --port 6380 --loglevel} err
assert_match {*'loglevel'*wrong number of arguments*} $err
# Take `6379` and `6380` as the port option value.
catch {exec src/redis-server --port 6379 6380} err
assert_match {*'port "6379" "6380"'*wrong number of arguments*} $err
# Take `--loglevel` and `verbose` as the port option value.
catch {exec src/redis-server --port --loglevel verbose} err
assert_match {*'port "--loglevel" "verbose"'*wrong number of arguments*} $err
# Take `--bla` as the port option value.
catch {exec src/redis-server --port --bla --loglevel verbose} err
assert_match {*'port "--bla"'*argument couldn't be parsed into an integer*} $err
# Take `--bla` as the loglevel option value.
catch {exec src/redis-server --logfile --my--log--file --loglevel --bla} err
assert_match {*'loglevel "--bla"'*argument(s) must be one of the following*} $err
# Using MULTI_ARG's own check, empty option value
catch {exec src/redis-server --shutdown-on-sigint} err
assert_match {*'shutdown-on-sigint'*argument(s) must be one of the following*} $err
catch {exec src/redis-server --shutdown-on-sigint "now force" --shutdown-on-sigterm} err
assert_match {*'shutdown-on-sigterm'*argument(s) must be one of the following*} $err
# Something like `redis-server --some-config --config-value1 --config-value2 --loglevel debug` would break,
# because if you want to pass a value to a config starting with `--`, it can only be a single value.
catch {exec src/redis-server --replicaof 127.0.0.1 abc} err
assert_match {*'replicaof "127.0.0.1" "abc"'*Invalid master port*} $err
catch {exec src/redis-server --replicaof --127.0.0.1 abc} err
assert_match {*'replicaof "--127.0.0.1" "abc"'*Invalid master port*} $err
catch {exec src/redis-server --replicaof --127.0.0.1 --abc} err
assert_match {*'replicaof "--127.0.0.1"'*wrong number of arguments*} $err
} {} {external:skip}
test {redis-server command line arguments - allow option value to use the `--` prefix} {
start_server {config "default.conf" args {--proc-title-template --my--title--template --loglevel verbose}} {
assert_match [r config get proc-title-template] {proc-title-template --my--title--template}
assert_match [r config get loglevel] {loglevel verbose}
}
} {} {external:skip}
test {redis-server command line arguments - save with empty input} {
# Take `--loglevel` as the save option value.
catch {exec src/redis-server --save --loglevel verbose} err
assert_match {*'save "--loglevel" "verbose"'*Invalid save parameters*} $err
start_server {config "default.conf" args {--save {} --loglevel verbose}} {
assert_match [r config get save] {save {}}
assert_match [r config get loglevel] {loglevel verbose}
}
} {} {external:skip}
test {redis-server command line arguments - take one bulk string with spaces for MULTI_ARG configs parsing} {
start_server {config "default.conf" args {--shutdown-on-sigint nosave force now --shutdown-on-sigterm "nosave force"}} {
assert_match [r config get shutdown-on-sigint] {shutdown-on-sigint {nosave now force}}
assert_match [r config get shutdown-on-sigterm] {shutdown-on-sigterm {nosave force}}
}
} {} {external:skip}
# Config file at this point is at a weird state, and includes all
# known keywords. Might be a good idea to avoid adding tests here.
}