diff --git a/src/config.c b/src/config.c index 0d435fecb..f7252ca6e 100644 --- a/src/config.c +++ b/src/config.c @@ -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. */ static sds configEnumGetName(configEnum *ce, int values, int bitflags) { sds names = NULL; - int matches = 0; + int unmatched = values; for( ; ce->name != NULL; ce++) { if (values == ce->val) { /* Short path for perfect match */ sdsfree(names); 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); - matches |= ce->val; + unmatched &= ~ce->val; } } - if (!names || values != matches) { + if (!names || unmatched) { sdsfree(names); return sdsnew("unknown"); } diff --git a/src/module.c b/src/module.c index 4bdaecb8a..3ef8e6f0a 100644 --- a/src/module.c +++ b/src/module.c @@ -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); - * + * + * 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. */ 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; diff --git a/tests/modules/moduleconfigs.c b/tests/modules/moduleconfigs.c index 0a6380461..b48133372 100644 --- a/tests/modules/moduleconfigs.c +++ b/tests/modules/moduleconfigs.c @@ -121,13 +121,13 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) } /* On the stack to make sure we're copying them. */ - const char *enum_vals[] = {"none", "one", "two", "three"}; - const int int_vals[] = {0, 1, 2, 4}; + const char *enum_vals[] = {"none", "five", "one", "two", "four"}; + 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; } - 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; } /* Memory config here. */ diff --git a/tests/unit/moduleapi/moduleconfigs.tcl b/tests/unit/moduleapi/moduleconfigs.tcl index f731d1bd9..8ebce3514 100644 --- a/tests/unit/moduleapi/moduleconfigs.tcl +++ b/tests/unit/moduleapi/moduleconfigs.tcl @@ -32,12 +32,27 @@ start_server {tags {"modules"}} { assert_equal [r config get moduleconfigs.enum] "moduleconfigs.enum two" r config set 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 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} { # Configs flagged immutable should not allow sets catch {[r config set moduleconfigs.immutable_bool yes]} e @@ -58,7 +73,7 @@ start_server {tags {"modules"}} { 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 - 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 } @@ -147,7 +162,7 @@ start_server {tags {"modules"}} { r config set moduleconfigs.mutable_bool yes r config set moduleconfigs.memory_numeric 750 r config set moduleconfigs.enum two - r config set moduleconfigs.flags "two three" + r config set moduleconfigs.flags "four two" r config rewrite restart_server 0 true false # Ensure configs we rewrote are present and that the conf file is readable @@ -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.string] "moduleconfigs.string {super \0secret password}" 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" r module unload moduleconfigs } @@ -230,12 +245,12 @@ start_server {tags {"modules"}} { 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 - 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.mutable_bool] "moduleconfigs.mutable_bool yes" 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.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.memory_numeric] "moduleconfigs.memory_numeric 1024" }