From 2bcd890d8aa645cab0d8fd0ed765c52a997de4f5 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Mon, 9 May 2022 13:37:49 +0300 Subject: [PATCH] Fix --save command line regression in redis 7.0.0 (#10690) Unintentional change in #9644 (since RC1) meant that an empty `--save ""` config from command line, wouldn't have clear any setting from the config file Added tests to cover that, and improved test infra to take additional command line args for redis-server --- src/config.c | 4 +++- tests/assets/default.conf | 3 +-- tests/support/server.tcl | 22 ++++++++++++++++------ tests/unit/introspection.tcl | 17 ++++++++++++++++- 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/config.c b/src/config.c index f7252ca6e..04f0dbcd8 100644 --- a/src/config.c +++ b/src/config.c @@ -2595,8 +2595,10 @@ static int setConfigSaveOption(standardConfig *config, sds *argv, int argc, cons int j; /* Special case: treat single arg "" as zero args indicating empty save configuration */ - if (argc == 1 && !strcasecmp(argv[0],"")) + if (argc == 1 && !strcasecmp(argv[0],"")) { + resetServerSaveParams(); argc = 0; + } /* Perform sanity check before setting the new config: * - Even number of args diff --git a/tests/assets/default.conf b/tests/assets/default.conf index 227ab5e7f..4ae420790 100644 --- a/tests/assets/default.conf +++ b/tests/assets/default.conf @@ -13,9 +13,8 @@ databases 16 latency-monitor-threshold 1 repl-diskless-sync-delay 0 +# Note the infrastructure in server.tcl uses a dict, we can't provide several save directives save 900 1 -save 300 10 -save 60 10000 rdbcompression yes dbfilename dump.rdb diff --git a/tests/support/server.tcl b/tests/support/server.tcl index 9d0c4510d..4a993f552 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -262,16 +262,22 @@ proc create_server_config_file {filename config} { close $fp } -proc spawn_server {config_file stdout stderr} { +proc spawn_server {config_file stdout stderr args} { + set cmd [list src/redis-server $config_file] + set args {*}$args + if {[llength $args] > 0} { + lappend cmd {*}$args + } + if {$::valgrind} { - set pid [exec valgrind --track-origins=yes --trace-children=yes --suppressions=[pwd]/src/valgrind.sup --show-reachable=no --show-possibly-lost=no --leak-check=full src/redis-server $config_file >> $stdout 2>> $stderr &] + set pid [exec valgrind --track-origins=yes --trace-children=yes --suppressions=[pwd]/src/valgrind.sup --show-reachable=no --show-possibly-lost=no --leak-check=full {*}$cmd >> $stdout 2>> $stderr &] } elseif ($::stack_logging) { - set pid [exec /usr/bin/env MallocStackLogging=1 MallocLogFile=/tmp/malloc_log.txt src/redis-server $config_file >> $stdout 2>> $stderr &] + set pid [exec /usr/bin/env MallocStackLogging=1 MallocLogFile=/tmp/malloc_log.txt {*}$cmd >> $stdout 2>> $stderr &] } else { # ASAN_OPTIONS environment variable is for address sanitizer. If a test # tries to allocate huge memory area and expects allocator to return # NULL, address sanitizer throws an error without this setting. - set pid [exec /usr/bin/env ASAN_OPTIONS=allocator_may_return_null=1 src/redis-server $config_file >> $stdout 2>> $stderr &] + set pid [exec /usr/bin/env ASAN_OPTIONS=allocator_may_return_null=1 {*}$cmd >> $stdout 2>> $stderr &] } if {$::wait_server} { @@ -398,6 +404,7 @@ proc start_server {options {code undefined}} { set overrides {} set omit {} set tags {} + set args {} set keep_persistence false # parse options @@ -409,6 +416,9 @@ proc start_server {options {code undefined}} { "overrides" { set overrides $value } + "args" { + set args $value + } "omit" { set omit $value } @@ -518,7 +528,7 @@ proc start_server {options {code undefined}} { send_data_packet $::test_server_fd "server-spawning" "port $port" - set pid [spawn_server $config_file $stdout $stderr] + set pid [spawn_server $config_file $stdout $stderr $args] # check that the server actually started set port_busy [wait_server_started $config_file $stdout $pid] @@ -721,7 +731,7 @@ proc restart_server {level wait_ready rotate_logs {reconnect 1} {shutdown sigter set config_file [dict get $srv "config_file"] - set pid [spawn_server $config_file $stdout $stderr] + set pid [spawn_server $config_file $stdout $stderr {}] # check that the server actually started wait_server_started $config_file $stdout $pid diff --git a/tests/unit/introspection.tcl b/tests/unit/introspection.tcl index 4b4b8f56b..41d20e20a 100644 --- a/tests/unit/introspection.tcl +++ b/tests/unit/introspection.tcl @@ -166,11 +166,26 @@ start_server {tags {"introspection"}} { assert_match [r config get save] {save {3600 1 300 100 60 10000}} } - # First "save" keyword overrides defaults + # First "save" keyword overrides hard coded defaults start_server {config "minimal.conf" overrides {save {100 100}}} { # Defaults assert_match [r config get save] {save {100 100}} } + + # First "save" keyword in default config file + start_server {config "default.conf"} { + assert_match [r config get save] {save {900 1}} + } + + # First "save" keyword appends default from config file + start_server {config "default.conf" args {--save 100 100}} { + assert_match [r config get save] {save {900 1 100 100}} + } + + # Empty "save" keyword resets all + start_server {config "default.conf" args {--save {}}} { + assert_match [r config get save] {save {}} + } } {} {external:skip} test {CONFIG sanity} {