Unverified Commit a09bc504 authored by Wen Hui's avatar Wen Hui Committed by GitHub
Browse files

Error message improvement for CONFIG SET command (#9924)

When CONFIG SET fails, print the name of the config that failed.
This is helpful since config set is now variadic.

however, there are cases where several configs have the same apply
function, and we can't be sure which one of them caused the failure.
parent c7dc17fc
...@@ -674,6 +674,8 @@ static void restoreBackupConfig(standardConfig **set_configs, sds *old_values, i ...@@ -674,6 +674,8 @@ static void restoreBackupConfig(standardConfig **set_configs, sds *old_values, i
void configSetCommand(client *c) { void configSetCommand(client *c) {
const char *errstr = NULL; const char *errstr = NULL;
const char *invalid_arg_name = NULL;
const char *err_arg_name = NULL;
standardConfig **set_configs; /* TODO: make this a dict for better performance */ standardConfig **set_configs; /* TODO: make this a dict for better performance */
sds *new_values; sds *new_values;
sds *old_values = NULL; sds *old_values = NULL;
...@@ -712,6 +714,7 @@ void configSetCommand(client *c) { ...@@ -712,6 +714,7 @@ void configSetCommand(client *c) {
if (config->flags & IMMUTABLE_CONFIG) { if (config->flags & IMMUTABLE_CONFIG) {
/* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */ /* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
errstr = "can't set immutable config"; errstr = "can't set immutable config";
err_arg_name = c->argv[2+i*2]->ptr;
invalid_args = 1; invalid_args = 1;
} }
...@@ -720,6 +723,7 @@ void configSetCommand(client *c) { ...@@ -720,6 +723,7 @@ void configSetCommand(client *c) {
if (set_configs[j] == config) { if (set_configs[j] == config) {
/* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */ /* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
errstr = "duplicate parameter"; errstr = "duplicate parameter";
err_arg_name = c->argv[2+i*2]->ptr;
invalid_args = 1; invalid_args = 1;
break; break;
} }
...@@ -733,7 +737,7 @@ void configSetCommand(client *c) { ...@@ -733,7 +737,7 @@ void configSetCommand(client *c) {
/* Fail if we couldn't find this config */ /* Fail if we couldn't find this config */
/* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */ /* Note: we don't abort the loop since we still want to handle redacting sensitive configs (above) */
if (!invalid_args && !set_configs[i]) { if (!invalid_args && !set_configs[i]) {
errstr = "unrecognized parameter"; invalid_arg_name = c->argv[2+i*2]->ptr;
invalid_args = 1; invalid_args = 1;
} }
} }
...@@ -749,6 +753,7 @@ void configSetCommand(client *c) { ...@@ -749,6 +753,7 @@ void configSetCommand(client *c) {
int res = performInterfaceSet(set_configs[i], new_values[i], &errstr); int res = performInterfaceSet(set_configs[i], new_values[i], &errstr);
if (!res) { if (!res) {
restoreBackupConfig(set_configs, old_values, i+1, NULL); restoreBackupConfig(set_configs, old_values, i+1, NULL);
err_arg_name = set_configs[i]->name;
goto err; goto err;
} else if (res == 1) { } else if (res == 1) {
/* A new value was set, if this config has an apply function then store it for execution later */ /* A new value was set, if this config has an apply function then store it for execution later */
...@@ -775,6 +780,7 @@ void configSetCommand(client *c) { ...@@ -775,6 +780,7 @@ void configSetCommand(client *c) {
if (!apply_fns[i](&errstr)) { if (!apply_fns[i](&errstr)) {
serverLog(LL_WARNING, "Failed applying new configuration. Possibly related to new %s setting. Restoring previous settings.", set_configs[config_map_fns[i]]->name); serverLog(LL_WARNING, "Failed applying new configuration. Possibly related to new %s setting. Restoring previous settings.", set_configs[config_map_fns[i]]->name);
restoreBackupConfig(set_configs, old_values, config_count, apply_fns); restoreBackupConfig(set_configs, old_values, config_count, apply_fns);
err_arg_name = set_configs[config_map_fns[i]]->name;
goto err; goto err;
} }
} }
...@@ -782,10 +788,12 @@ void configSetCommand(client *c) { ...@@ -782,10 +788,12 @@ void configSetCommand(client *c) {
goto end; goto end;
err: err:
if (errstr) { if (invalid_arg_name) {
addReplyErrorFormat(c,"Config set failed - %s", errstr); addReplyErrorFormat(c,"Unknown option or number of arguments for CONFIG SET - '%s'", invalid_arg_name);
} else if (errstr) {
addReplyErrorFormat(c,"CONFIG SET failed (possibly related to argument '%s') - %s", err_arg_name, errstr);
} else { } else {
addReplyError(c,"Invalid arguments"); addReplyErrorFormat(c,"CONFIG SET failed (possibly related to argument '%s')", err_arg_name);
} }
end: end:
zfree(set_configs); zfree(set_configs);
......
...@@ -322,7 +322,7 @@ start_server {tags {"introspection"}} { ...@@ -322,7 +322,7 @@ start_server {tags {"introspection"}} {
# Set some value to maxmemory # Set some value to maxmemory
assert_equal [r config set maxmemory 10000002] "OK" assert_equal [r config set maxmemory 10000002] "OK"
# Set another value to maxmeory together with another invalid config # Set another value to maxmeory together with another invalid config
assert_error "ERR Config set failed - percentage argument must be less or equal to 100" { assert_error "ERR CONFIG SET failed (possibly related to argument 'maxmemory-clients') - percentage argument must be less or equal to 100" {
r config set maxmemory 10000001 maxmemory-clients 200% client-query-buffer-limit invalid r config set maxmemory 10000001 maxmemory-clients 200% client-query-buffer-limit invalid
} }
# Validate we rolled back to original values # Validate we rolled back to original values
...@@ -375,7 +375,7 @@ start_server {tags {"introspection"}} { ...@@ -375,7 +375,7 @@ start_server {tags {"introspection"}} {
# Try to listen on the used port, pass some more configs to make sure the # Try to listen on the used port, pass some more configs to make sure the
# returned failure message is for the first bad config and everything is rolled back. # returned failure message is for the first bad config and everything is rolled back.
assert_error "ERR Config set failed - Unable to listen on this port*" { assert_error "ERR CONFIG SET failed (possibly related to argument 'port') - Unable to listen on this port*" {
eval "r config set $some_configs" eval "r config set $some_configs"
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment