Unverified Commit eb915a82 authored by Oran Agra's avatar Oran Agra Committed by GitHub
Browse files

Bug fixes for enum configs with overlapping bit flags (module API) (#10661)

If we want to support bits that can be overlapping, we need to make sure
that:
1. we don't use the same bit for two return values.
2. values should be sorted so that prefer ones (matching more
   bits) come first.
parent 8fc95921
...@@ -311,18 +311,21 @@ int configEnumGetValue(configEnum *ce, sds *argv, int argc, int bitflags) { ...@@ -311,18 +311,21 @@ int configEnumGetValue(configEnum *ce, sds *argv, int argc, int bitflags) {
/* Get enum name/s from value. If no matches are found "unknown" is returned. */ /* Get enum name/s from value. If no matches are found "unknown" is returned. */
static sds configEnumGetName(configEnum *ce, int values, int bitflags) { static sds configEnumGetName(configEnum *ce, int values, int bitflags) {
sds names = NULL; sds names = NULL;
int matches = 0; int unmatched = values;
for( ; ce->name != NULL; ce++) { for( ; ce->name != NULL; ce++) {
if (values == ce->val) { /* Short path for perfect match */ if (values == ce->val) { /* Short path for perfect match */
sdsfree(names); sdsfree(names);
return sdsnew(ce->name); return sdsnew(ce->name);
} }
if (bitflags && (values & ce->val)) {
/* Note: for bitflags, we want them sorted from high to low, so that if there are several / partially
* overlapping entries, we'll prefer the ones matching more bits. */
if (bitflags && ce->val && ce->val == (unmatched & ce->val)) {
names = names ? sdscatfmt(names, " %s", ce->name) : sdsnew(ce->name); names = names ? sdscatfmt(names, " %s", ce->name) : sdsnew(ce->name);
matches |= ce->val; unmatched &= ~ce->val;
} }
} }
if (!names || values != matches) { if (!names || unmatched) {
sdsfree(names); sdsfree(names);
return sdsnew("unknown"); return sdsnew("unknown");
} }
......
...@@ -11679,7 +11679,11 @@ int RM_RegisterBoolConfig(RedisModuleCtx *ctx, const char *name, int default_val ...@@ -11679,7 +11679,11 @@ int RM_RegisterBoolConfig(RedisModuleCtx *ctx, const char *name, int default_val
* } * }
* ... * ...
* RedisModule_RegisterEnumConfig(ctx, "enum", 0, REDISMODULE_CONFIG_DEFAULT, enum_vals, int_vals, 3, getEnumConfigCommand, setEnumConfigCommand, NULL, NULL); * RedisModule_RegisterEnumConfig(ctx, "enum", 0, REDISMODULE_CONFIG_DEFAULT, enum_vals, int_vals, 3, getEnumConfigCommand, setEnumConfigCommand, NULL, NULL);
* *
* Note that you can use REDISMODULE_CONFIG_BITFLAGS so that multiple enum string
* can be combined into one integer as bit flags, in which case you may want to
* sort your enums so that the preferred combinations are present first.
*
* See RedisModule_RegisterStringConfig for detailed general information about configs. */ * See RedisModule_RegisterStringConfig for detailed general information about configs. */
int RM_RegisterEnumConfig(RedisModuleCtx *ctx, const char *name, int default_val, unsigned int flags, const char **enum_values, const int *int_values, int num_enum_vals, RedisModuleConfigGetEnumFunc getfn, RedisModuleConfigSetEnumFunc setfn, RedisModuleConfigApplyFunc applyfn, void *privdata) { int RM_RegisterEnumConfig(RedisModuleCtx *ctx, const char *name, int default_val, unsigned int flags, const char **enum_values, const int *int_values, int num_enum_vals, RedisModuleConfigGetEnumFunc getfn, RedisModuleConfigSetEnumFunc setfn, RedisModuleConfigApplyFunc applyfn, void *privdata) {
RedisModule *module = ctx->module; RedisModule *module = ctx->module;
......
...@@ -121,13 +121,13 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) ...@@ -121,13 +121,13 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
} }
/* On the stack to make sure we're copying them. */ /* On the stack to make sure we're copying them. */
const char *enum_vals[] = {"none", "one", "two", "three"}; const char *enum_vals[] = {"none", "five", "one", "two", "four"};
const int int_vals[] = {0, 1, 2, 4}; const int int_vals[] = {0, 5, 1, 2, 4};
if (RedisModule_RegisterEnumConfig(ctx, "enum", 1, REDISMODULE_CONFIG_DEFAULT, enum_vals, int_vals, 4, getEnumConfigCommand, setEnumConfigCommand, NULL, NULL) == REDISMODULE_ERR) { if (RedisModule_RegisterEnumConfig(ctx, "enum", 1, REDISMODULE_CONFIG_DEFAULT, enum_vals, int_vals, 5, getEnumConfigCommand, setEnumConfigCommand, NULL, NULL) == REDISMODULE_ERR) {
return REDISMODULE_ERR; return REDISMODULE_ERR;
} }
if (RedisModule_RegisterEnumConfig(ctx, "flags", 3, REDISMODULE_CONFIG_DEFAULT | REDISMODULE_CONFIG_BITFLAGS, enum_vals, int_vals, 4, getFlagsConfigCommand, setFlagsConfigCommand, NULL, NULL) == REDISMODULE_ERR) { if (RedisModule_RegisterEnumConfig(ctx, "flags", 3, REDISMODULE_CONFIG_DEFAULT | REDISMODULE_CONFIG_BITFLAGS, enum_vals, int_vals, 5, getFlagsConfigCommand, setFlagsConfigCommand, NULL, NULL) == REDISMODULE_ERR) {
return REDISMODULE_ERR; return REDISMODULE_ERR;
} }
/* Memory config here. */ /* Memory config here. */
......
...@@ -32,12 +32,27 @@ start_server {tags {"modules"}} { ...@@ -32,12 +32,27 @@ start_server {tags {"modules"}} {
assert_equal [r config get moduleconfigs.enum] "moduleconfigs.enum two" assert_equal [r config get moduleconfigs.enum] "moduleconfigs.enum two"
r config set moduleconfigs.flags two r config set moduleconfigs.flags two
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags two" assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags two"
r config set moduleconfigs.flags "two three"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags {two three}"
r config set moduleconfigs.numeric -2 r config set moduleconfigs.numeric -2
assert_equal [r config get moduleconfigs.numeric] "moduleconfigs.numeric -2" assert_equal [r config get moduleconfigs.numeric] "moduleconfigs.numeric -2"
} }
test {Config set commands enum flags} {
r config set moduleconfigs.flags "none"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags none"
r config set moduleconfigs.flags "two four"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags {two four}"
r config set moduleconfigs.flags "five"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags five"
r config set moduleconfigs.flags "one four"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags five"
r config set moduleconfigs.flags "one two four"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags {five two}"
}
test {Immutable flag works properly and rejected strings dont leak} { test {Immutable flag works properly and rejected strings dont leak} {
# Configs flagged immutable should not allow sets # Configs flagged immutable should not allow sets
catch {[r config set moduleconfigs.immutable_bool yes]} e catch {[r config set moduleconfigs.immutable_bool yes]} e
...@@ -58,7 +73,7 @@ start_server {tags {"modules"}} { ...@@ -58,7 +73,7 @@ start_server {tags {"modules"}} {
test {Enums only able to be set to passed in values} { test {Enums only able to be set to passed in values} {
# Module authors specify what values are valid for enums, check that only those values are ok on a set # Module authors specify what values are valid for enums, check that only those values are ok on a set
catch {[r config set moduleconfigs.enum four]} e catch {[r config set moduleconfigs.enum asdf]} e
assert_match {*must be one of the following*} $e assert_match {*must be one of the following*} $e
} }
...@@ -147,7 +162,7 @@ start_server {tags {"modules"}} { ...@@ -147,7 +162,7 @@ start_server {tags {"modules"}} {
r config set moduleconfigs.mutable_bool yes r config set moduleconfigs.mutable_bool yes
r config set moduleconfigs.memory_numeric 750 r config set moduleconfigs.memory_numeric 750
r config set moduleconfigs.enum two r config set moduleconfigs.enum two
r config set moduleconfigs.flags "two three" r config set moduleconfigs.flags "four two"
r config rewrite r config rewrite
restart_server 0 true false restart_server 0 true false
# Ensure configs we rewrote are present and that the conf file is readable # Ensure configs we rewrote are present and that the conf file is readable
...@@ -155,7 +170,7 @@ start_server {tags {"modules"}} { ...@@ -155,7 +170,7 @@ start_server {tags {"modules"}} {
assert_equal [r config get moduleconfigs.memory_numeric] "moduleconfigs.memory_numeric 750" assert_equal [r config get moduleconfigs.memory_numeric] "moduleconfigs.memory_numeric 750"
assert_equal [r config get moduleconfigs.string] "moduleconfigs.string {super \0secret password}" assert_equal [r config get moduleconfigs.string] "moduleconfigs.string {super \0secret password}"
assert_equal [r config get moduleconfigs.enum] "moduleconfigs.enum two" assert_equal [r config get moduleconfigs.enum] "moduleconfigs.enum two"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags {two three}" assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags {two four}"
assert_equal [r config get moduleconfigs.numeric] "moduleconfigs.numeric -1" assert_equal [r config get moduleconfigs.numeric] "moduleconfigs.numeric -1"
r module unload moduleconfigs r module unload moduleconfigs
} }
...@@ -230,12 +245,12 @@ start_server {tags {"modules"}} { ...@@ -230,12 +245,12 @@ start_server {tags {"modules"}} {
set stdout [dict get $noload stdout] set stdout [dict get $noload stdout]
assert_equal [count_message_lines $stdout "Module Configurations were not set, likely a missing LoadConfigs call. Unloading the module."] 1 assert_equal [count_message_lines $stdout "Module Configurations were not set, likely a missing LoadConfigs call. Unloading the module."] 1
start_server [list overrides [list loadmodule "$testmodule" moduleconfigs.string "bootedup" moduleconfigs.enum two moduleconfigs.flags "two three"]] { start_server [list overrides [list loadmodule "$testmodule" moduleconfigs.string "bootedup" moduleconfigs.enum two moduleconfigs.flags "two four"]] {
assert_equal [r config get moduleconfigs.string] "moduleconfigs.string bootedup" assert_equal [r config get moduleconfigs.string] "moduleconfigs.string bootedup"
assert_equal [r config get moduleconfigs.mutable_bool] "moduleconfigs.mutable_bool yes" assert_equal [r config get moduleconfigs.mutable_bool] "moduleconfigs.mutable_bool yes"
assert_equal [r config get moduleconfigs.immutable_bool] "moduleconfigs.immutable_bool no" assert_equal [r config get moduleconfigs.immutable_bool] "moduleconfigs.immutable_bool no"
assert_equal [r config get moduleconfigs.enum] "moduleconfigs.enum two" assert_equal [r config get moduleconfigs.enum] "moduleconfigs.enum two"
assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags {two three}" assert_equal [r config get moduleconfigs.flags] "moduleconfigs.flags {two four}"
assert_equal [r config get moduleconfigs.numeric] "moduleconfigs.numeric -1" assert_equal [r config get moduleconfigs.numeric] "moduleconfigs.numeric -1"
assert_equal [r config get moduleconfigs.memory_numeric] "moduleconfigs.memory_numeric 1024" assert_equal [r config get moduleconfigs.memory_numeric] "moduleconfigs.memory_numeric 1024"
} }
......
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