From 26d0fe18cf96b7118bea47975d54785097c2ac0e Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 25 Sep 2025 13:56:56 +0800 Subject: [PATCH 1/8] Make commandlog-request-larger-than and commandlog-reply-larger-than become memory config Signed-off-by: Binbin --- src/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.c b/src/config.c index e799e24ffc9..993539c22d4 100644 --- a/src/config.c +++ b/src/config.c @@ -3364,8 +3364,8 @@ standardConfig static_configs[] = { createLongLongConfig("cluster-node-timeout", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.cluster_node_timeout, 15000, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("cluster-ping-interval", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, 0, LLONG_MAX, server.cluster_ping_interval, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("commandlog-execution-slower-than", "slowlog-log-slower-than", MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_SLOW].threshold, 10000, INTEGER_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, INTEGER_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, INTEGER_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), createLongLongConfig("latency-monitor-threshold", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.latency_monitor_threshold, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("proto-max-bulk-len", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.proto_max_bulk_len, 512ll * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Bulk request max size */ createLongLongConfig("stream-node-max-entries", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.stream_node_max_entries, 100, INTEGER_CONFIG, NULL, NULL), From 82ef54c22b1b586d39f3265c431eb998f6123877 Mon Sep 17 00:00:00 2001 From: Binbin Date: Sat, 25 Oct 2025 16:52:04 +0800 Subject: [PATCH 2/8] Add MEMORY_SUPPORT_NEGATIVE_CONFIG to support negative number Signed-off-by: Binbin --- src/config.c | 23 +++++++++++++-- src/server.h | 10 ++++--- src/util.c | 1 - tests/unit/commandlog.tcl | 61 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 7 deletions(-) diff --git a/src/config.c b/src/config.c index 993539c22d4..7fc8d26e468 100644 --- a/src/config.c +++ b/src/config.c @@ -2154,6 +2154,15 @@ static int numericParseString(standardConfig *config, sds value, const char **er if (!memerr) return 1; } + /* First try to parse as memory, and check if it supports negative numbers. */ + if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) { + int memerr; + *res = memtoull(value, &memerr); + if (!memerr) return 1; + + if (string2ll(value, sdslen(value), res)) return 1; + } + /* Attempt to parse as percent */ if (config->data.numeric.flags & PERCENT_CONFIG && sdslen(value) > 1 && value[sdslen(value) - 1] == '%' && string2ll(value, sdslen(value) - 1, res) && *res >= 0) { @@ -2178,6 +2187,8 @@ static int numericParseString(standardConfig *config, sds value, const char **er *err = "argument must be a memory or percent value"; else if (config->data.numeric.flags & MEMORY_CONFIG) *err = "argument must be a memory value"; + else if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) + *err = "argument must be a memory value or couldn't be parsed into an integer"; else if (config->data.numeric.flags & OCTAL_CONFIG) *err = "argument couldn't be parsed as an octal number"; else @@ -2215,6 +2226,8 @@ static sds numericConfigGet(standardConfig *config) { buf[len + 1] = '\0'; } else if (config->data.numeric.flags & MEMORY_CONFIG) { ull2string(buf, sizeof(buf), value); + } else if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) { + ll2string(buf, sizeof(buf), value); } else if (config->data.numeric.flags & OCTAL_CONFIG) { snprintf(buf, sizeof(buf), "%llo", value); } else { @@ -2232,6 +2245,12 @@ static void numericConfigRewrite(standardConfig *config, const char *name, struc rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); + } else if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) { + if (value >= 0) { + rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); + } else { + rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value); + } } else if (config->data.numeric.flags & OCTAL_CONFIG) { rewriteConfigOctalOption(state, name, value, config->data.numeric.default_value); } else { @@ -3364,8 +3383,8 @@ standardConfig static_configs[] = { createLongLongConfig("cluster-node-timeout", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.cluster_node_timeout, 15000, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("cluster-ping-interval", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, 0, LLONG_MAX, server.cluster_ping_interval, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("commandlog-execution-slower-than", "slowlog-log-slower-than", MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_SLOW].threshold, 10000, INTEGER_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, MEMORY_SUPPORT_NEGATIVE_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, MEMORY_SUPPORT_NEGATIVE_CONFIG, NULL, NULL), createLongLongConfig("latency-monitor-threshold", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.latency_monitor_threshold, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("proto-max-bulk-len", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.proto_max_bulk_len, 512ll * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Bulk request max size */ createLongLongConfig("stream-node-max-entries", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.stream_node_max_entries, 100, INTEGER_CONFIG, NULL, NULL), diff --git a/src/server.h b/src/server.h index ed455c08f3f..86b8ac74aca 100644 --- a/src/server.h +++ b/src/server.h @@ -3496,10 +3496,12 @@ sds keyspaceEventsFlagsToString(int flags); * to apply the configuration change even if the new config value is the same as \ * the old. */ -#define INTEGER_CONFIG 0 /* No flags means a simple integer configuration */ -#define MEMORY_CONFIG (1 << 0) /* Indicates if this value can be loaded as a memory value */ -#define PERCENT_CONFIG (1 << 1) /* Indicates if this value can be loaded as a percent (and stored as a negative int) */ -#define OCTAL_CONFIG (1 << 2) /* This value uses octal representation */ +#define INTEGER_CONFIG 0 /* No flags means a simple integer configuration */ +#define MEMORY_CONFIG (1 << 0) /* Indicates if this value can be loaded as a memory value */ +#define PERCENT_CONFIG (1 << 1) /* Indicates if this value can be loaded as a percent (and stored as a negative int) */ +#define OCTAL_CONFIG (1 << 2) /* This value uses octal representation */ +#define MEMORY_SUPPORT_NEGATIVE_CONFIG (1 << 3) /* Indicates if this value can be loaded as a memory value and support negative number */ + /* Enum Configs contain an array of configEnum objects that match a string with an integer. */ typedef struct configEnum { diff --git a/src/util.c b/src/util.c index 0631736c86d..3f123314a19 100644 --- a/src/util.c +++ b/src/util.c @@ -388,7 +388,6 @@ int ull2string(char *dst, size_t dstlen, unsigned long long value) { /* Check length. */ uint32_t length = digits10(value); if (length >= dstlen) goto err; - ; /* Null term. */ uint32_t next = length - 1; diff --git a/tests/unit/commandlog.tcl b/tests/unit/commandlog.tcl index cd5c518567f..5b8cafdc3be 100644 --- a/tests/unit/commandlog.tcl +++ b/tests/unit/commandlog.tcl @@ -363,4 +363,65 @@ start_server {tags {"commandlog"} overrides {commandlog-execution-slower-than 10 assert_equal {test-client} [lindex $ping_cmd 5] } } + + test {COMMANDLOG - special number -1 disables the command logging} { + r config set commandlog-execution-slower-than -1 + r config set commandlog-request-larger-than -1 + r config set commandlog-reply-larger-than -1 + + r commandlog reset slow + r commandlog reset large-request + r commandlog reset large-reply + + r ping + assert_equal [r commandlog len slow] 0 + assert_equal [r commandlog len large-request] 0 + assert_equal [r commandlog len large-reply] 0 + } + + test {COMMANDLOG - special number 0 logs every command} { + r config set commandlog-execution-slower-than 0 + r commandlog reset slow + r ping + set slow_resp [r commandlog get -1 slow] + assert_equal 2 [llength $slowlog_resp] + assert_equal {ping} [lindex [lindex $slow_resp 0] 3] + assert_equal {commandlog reset slow} [lindex [lindex $slow_resp 1] 3] + + r config set commandlog-request-larger-than 0 + r commandlog reset large-request + r ping + set large_request_resp [r commandlog get -1 large-request] + assert_equal 2 [llength $large_request_resp] + assert_equal {ping} [lindex [lindex $large_request_resp 0] 3] + assert_equal {commandlog reset large-request} [lindex [lindex $large_request_resp 1] 3] + + r config set commandlog-reply-larger-than 0 + r commandlog reset large-reply + r ping + set large_reply_resp [r commandlog get -1 large-reply] + assert_equal 2 [llength $large_reply_resp] + assert_equal {ping} [lindex [lindex $large_reply_resp 0] 3] + assert_equal {commandlog reset large-reply} [lindex [lindex $large_reply_resp 1] 3] + } + + test {COMMANDLOG - config set / config get} { + r config set commandlog-request-larger-than 0 + r config set commandlog-reply-larger-than 0 + assert_equal [r config get commandlog-request-larger-than] {commandlog-request-larger-than 0} + assert_equal [r config get commandlog-reply-larger-than] {commandlog-reply-larger-than 0} + + r config set commandlog-request-larger-than -1 + r config set commandlog-reply-larger-than -1 + assert_equal [r config get commandlog-request-larger-than] {commandlog-request-larger-than -1} + assert_equal [r config get commandlog-reply-larger-than] {commandlog-reply-larger-than -1} + + r config set commandlog-request-larger-than 10MB + r config set commandlog-reply-larger-than 10MB + assert_equal [r config get commandlog-request-larger-than] {commandlog-request-larger-than 10485760} + assert_equal [r config get commandlog-reply-larger-than] {commandlog-reply-larger-than 10485760} + + assert_error {ERR *} {r config set commandlog-request-larger-than -100} + assert_error {ERR *} {r config set commandlog-reply-larger-than -100} + } } From 5d6e3f565561c4ae19b9ad01a2e5a924c9f5f743 Mon Sep 17 00:00:00 2001 From: Binbin Date: Sat, 25 Oct 2025 22:30:51 +0800 Subject: [PATCH 3/8] Remove MEMORY_SUPPORT_NEGATIVE_CONFIG just use MEMORY_CONFIG Signed-off-by: Binbin --- src/config.c | 17 ++--------------- src/server.h | 9 ++++----- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/src/config.c b/src/config.c index 7fc8d26e468..b23b55ba86e 100644 --- a/src/config.c +++ b/src/config.c @@ -2152,13 +2152,6 @@ static int numericParseString(standardConfig *config, sds value, const char **er int memerr; *res = memtoull(value, &memerr); if (!memerr) return 1; - } - - /* First try to parse as memory, and check if it supports negative numbers. */ - if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) { - int memerr; - *res = memtoull(value, &memerr); - if (!memerr) return 1; if (string2ll(value, sdslen(value), res)) return 1; } @@ -2186,8 +2179,6 @@ static int numericParseString(standardConfig *config, sds value, const char **er if (config->data.numeric.flags & MEMORY_CONFIG && config->data.numeric.flags & PERCENT_CONFIG) *err = "argument must be a memory or percent value"; else if (config->data.numeric.flags & MEMORY_CONFIG) - *err = "argument must be a memory value"; - else if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) *err = "argument must be a memory value or couldn't be parsed into an integer"; else if (config->data.numeric.flags & OCTAL_CONFIG) *err = "argument couldn't be parsed as an octal number"; @@ -2225,8 +2216,6 @@ static sds numericConfigGet(standardConfig *config) { buf[len] = '%'; buf[len + 1] = '\0'; } else if (config->data.numeric.flags & MEMORY_CONFIG) { - ull2string(buf, sizeof(buf), value); - } else if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) { ll2string(buf, sizeof(buf), value); } else if (config->data.numeric.flags & OCTAL_CONFIG) { snprintf(buf, sizeof(buf), "%llo", value); @@ -2244,8 +2233,6 @@ static void numericConfigRewrite(standardConfig *config, const char *name, struc if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) { rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { - rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); - } else if (config->data.numeric.flags & MEMORY_SUPPORT_NEGATIVE_CONFIG) { if (value >= 0) { rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); } else { @@ -3383,8 +3370,8 @@ standardConfig static_configs[] = { createLongLongConfig("cluster-node-timeout", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.cluster_node_timeout, 15000, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("cluster-ping-interval", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, 0, LLONG_MAX, server.cluster_ping_interval, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("commandlog-execution-slower-than", "slowlog-log-slower-than", MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_SLOW].threshold, 10000, INTEGER_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, MEMORY_SUPPORT_NEGATIVE_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, MEMORY_SUPPORT_NEGATIVE_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), createLongLongConfig("latency-monitor-threshold", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.latency_monitor_threshold, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("proto-max-bulk-len", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.proto_max_bulk_len, 512ll * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Bulk request max size */ createLongLongConfig("stream-node-max-entries", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.stream_node_max_entries, 100, INTEGER_CONFIG, NULL, NULL), diff --git a/src/server.h b/src/server.h index 86b8ac74aca..7887de113a4 100644 --- a/src/server.h +++ b/src/server.h @@ -3496,11 +3496,10 @@ sds keyspaceEventsFlagsToString(int flags); * to apply the configuration change even if the new config value is the same as \ * the old. */ -#define INTEGER_CONFIG 0 /* No flags means a simple integer configuration */ -#define MEMORY_CONFIG (1 << 0) /* Indicates if this value can be loaded as a memory value */ -#define PERCENT_CONFIG (1 << 1) /* Indicates if this value can be loaded as a percent (and stored as a negative int) */ -#define OCTAL_CONFIG (1 << 2) /* This value uses octal representation */ -#define MEMORY_SUPPORT_NEGATIVE_CONFIG (1 << 3) /* Indicates if this value can be loaded as a memory value and support negative number */ +#define INTEGER_CONFIG 0 /* No flags means a simple integer configuration */ +#define MEMORY_CONFIG (1 << 0) /* Indicates if this value can be loaded as a memory value */ +#define PERCENT_CONFIG (1 << 1) /* Indicates if this value can be loaded as a percent (and stored as a negative int) */ +#define OCTAL_CONFIG (1 << 2) /* This value uses octal representation */ /* Enum Configs contain an array of configEnum objects that match a string with an integer. */ From 6157bd9011bc0624533caf867c41d6e352368fca Mon Sep 17 00:00:00 2001 From: Binbin Date: Sat, 25 Oct 2025 22:33:40 +0800 Subject: [PATCH 4/8] Remove dummy line Signed-off-by: Binbin --- src/server.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/server.h b/src/server.h index 7887de113a4..ed455c08f3f 100644 --- a/src/server.h +++ b/src/server.h @@ -3501,7 +3501,6 @@ sds keyspaceEventsFlagsToString(int flags); #define PERCENT_CONFIG (1 << 1) /* Indicates if this value can be loaded as a percent (and stored as a negative int) */ #define OCTAL_CONFIG (1 << 2) /* This value uses octal representation */ - /* Enum Configs contain an array of configEnum objects that match a string with an integer. */ typedef struct configEnum { char *name; From b291c9fc93f9e0fed61005d6ed00984c42a9cbd1 Mon Sep 17 00:00:00 2001 From: Binbin Date: Mon, 27 Oct 2025 22:54:50 +0800 Subject: [PATCH 5/8] Try to migrate the unsigned problem Signed-off-by: Binbin --- src/config.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/config.c b/src/config.c index b23b55ba86e..283568e002f 100644 --- a/src/config.c +++ b/src/config.c @@ -1275,7 +1275,7 @@ int rewriteConfigRewriteLine(struct rewriteConfigState *state, const char *optio /* Write the long long 'bytes' value as a string in a way that is parsable * inside valkey.conf. If possible uses the GB, MB, KB notation. */ -int rewriteConfigFormatMemory(char *buf, size_t len, long long bytes) { +int rewriteConfigFormatMemory(char *buf, size_t len, unsigned long long bytes) { int gb = 1024 * 1024 * 1024; int mb = 1024 * 1024; int kb = 1024; @@ -1294,8 +1294,8 @@ int rewriteConfigFormatMemory(char *buf, size_t len, long long bytes) { /* Rewrite a simple "option-name " configuration option. */ void rewriteConfigBytesOption(struct rewriteConfigState *state, const char *option, - long long value, - long long defvalue) { + unsigned long long value, + unsigned long long defvalue) { char buf[64]; int force = value != defvalue; sds line; @@ -2216,7 +2216,11 @@ static sds numericConfigGet(standardConfig *config) { buf[len] = '%'; buf[len + 1] = '\0'; } else if (config->data.numeric.flags & MEMORY_CONFIG) { - ll2string(buf, sizeof(buf), value); + if (config->data.numeric.numeric_type == NUMERIC_TYPE_LONG_LONG && value < 0) { + ll2string(buf, sizeof(buf), value); + } else { + ull2string(buf, sizeof(buf), value); + } } else if (config->data.numeric.flags & OCTAL_CONFIG) { snprintf(buf, sizeof(buf), "%llo", value); } else { @@ -2233,10 +2237,10 @@ static void numericConfigRewrite(standardConfig *config, const char *name, struc if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) { rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { - if (value >= 0) { - rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); - } else { + if (config->data.numeric.numeric_type == NUMERIC_TYPE_LONG_LONG && value < 0) { rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value); + } else { + rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); } } else if (config->data.numeric.flags & OCTAL_CONFIG) { rewriteConfigOctalOption(state, name, value, config->data.numeric.default_value); From ffdd16942627dcc2e440f39ce10aa1d7978b440d Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 9 Apr 2026 10:57:14 +0800 Subject: [PATCH 6/8] Revert all the commits Signed-off-by: Binbin --- src/config.c | 26 +++++------------ src/util.c | 1 + tests/unit/commandlog.tcl | 61 --------------------------------------- 3 files changed, 9 insertions(+), 79 deletions(-) diff --git a/src/config.c b/src/config.c index 283568e002f..e799e24ffc9 100644 --- a/src/config.c +++ b/src/config.c @@ -1275,7 +1275,7 @@ int rewriteConfigRewriteLine(struct rewriteConfigState *state, const char *optio /* Write the long long 'bytes' value as a string in a way that is parsable * inside valkey.conf. If possible uses the GB, MB, KB notation. */ -int rewriteConfigFormatMemory(char *buf, size_t len, unsigned long long bytes) { +int rewriteConfigFormatMemory(char *buf, size_t len, long long bytes) { int gb = 1024 * 1024 * 1024; int mb = 1024 * 1024; int kb = 1024; @@ -1294,8 +1294,8 @@ int rewriteConfigFormatMemory(char *buf, size_t len, unsigned long long bytes) { /* Rewrite a simple "option-name " configuration option. */ void rewriteConfigBytesOption(struct rewriteConfigState *state, const char *option, - unsigned long long value, - unsigned long long defvalue) { + long long value, + long long defvalue) { char buf[64]; int force = value != defvalue; sds line; @@ -2152,8 +2152,6 @@ static int numericParseString(standardConfig *config, sds value, const char **er int memerr; *res = memtoull(value, &memerr); if (!memerr) return 1; - - if (string2ll(value, sdslen(value), res)) return 1; } /* Attempt to parse as percent */ @@ -2179,7 +2177,7 @@ static int numericParseString(standardConfig *config, sds value, const char **er if (config->data.numeric.flags & MEMORY_CONFIG && config->data.numeric.flags & PERCENT_CONFIG) *err = "argument must be a memory or percent value"; else if (config->data.numeric.flags & MEMORY_CONFIG) - *err = "argument must be a memory value or couldn't be parsed into an integer"; + *err = "argument must be a memory value"; else if (config->data.numeric.flags & OCTAL_CONFIG) *err = "argument couldn't be parsed as an octal number"; else @@ -2216,11 +2214,7 @@ static sds numericConfigGet(standardConfig *config) { buf[len] = '%'; buf[len + 1] = '\0'; } else if (config->data.numeric.flags & MEMORY_CONFIG) { - if (config->data.numeric.numeric_type == NUMERIC_TYPE_LONG_LONG && value < 0) { - ll2string(buf, sizeof(buf), value); - } else { - ull2string(buf, sizeof(buf), value); - } + ull2string(buf, sizeof(buf), value); } else if (config->data.numeric.flags & OCTAL_CONFIG) { snprintf(buf, sizeof(buf), "%llo", value); } else { @@ -2237,11 +2231,7 @@ static void numericConfigRewrite(standardConfig *config, const char *name, struc if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) { rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { - if (config->data.numeric.numeric_type == NUMERIC_TYPE_LONG_LONG && value < 0) { - rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value); - } else { - rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); - } + rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); } else if (config->data.numeric.flags & OCTAL_CONFIG) { rewriteConfigOctalOption(state, name, value, config->data.numeric.default_value); } else { @@ -3374,8 +3364,8 @@ standardConfig static_configs[] = { createLongLongConfig("cluster-node-timeout", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.cluster_node_timeout, 15000, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("cluster-ping-interval", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, 0, LLONG_MAX, server.cluster_ping_interval, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("commandlog-execution-slower-than", "slowlog-log-slower-than", MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_SLOW].threshold, 10000, INTEGER_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, MEMORY_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, INTEGER_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("latency-monitor-threshold", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.latency_monitor_threshold, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("proto-max-bulk-len", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.proto_max_bulk_len, 512ll * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Bulk request max size */ createLongLongConfig("stream-node-max-entries", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.stream_node_max_entries, 100, INTEGER_CONFIG, NULL, NULL), diff --git a/src/util.c b/src/util.c index 3f123314a19..0631736c86d 100644 --- a/src/util.c +++ b/src/util.c @@ -388,6 +388,7 @@ int ull2string(char *dst, size_t dstlen, unsigned long long value) { /* Check length. */ uint32_t length = digits10(value); if (length >= dstlen) goto err; + ; /* Null term. */ uint32_t next = length - 1; diff --git a/tests/unit/commandlog.tcl b/tests/unit/commandlog.tcl index 5b8cafdc3be..cd5c518567f 100644 --- a/tests/unit/commandlog.tcl +++ b/tests/unit/commandlog.tcl @@ -363,65 +363,4 @@ start_server {tags {"commandlog"} overrides {commandlog-execution-slower-than 10 assert_equal {test-client} [lindex $ping_cmd 5] } } - - test {COMMANDLOG - special number -1 disables the command logging} { - r config set commandlog-execution-slower-than -1 - r config set commandlog-request-larger-than -1 - r config set commandlog-reply-larger-than -1 - - r commandlog reset slow - r commandlog reset large-request - r commandlog reset large-reply - - r ping - assert_equal [r commandlog len slow] 0 - assert_equal [r commandlog len large-request] 0 - assert_equal [r commandlog len large-reply] 0 - } - - test {COMMANDLOG - special number 0 logs every command} { - r config set commandlog-execution-slower-than 0 - r commandlog reset slow - r ping - set slow_resp [r commandlog get -1 slow] - assert_equal 2 [llength $slowlog_resp] - assert_equal {ping} [lindex [lindex $slow_resp 0] 3] - assert_equal {commandlog reset slow} [lindex [lindex $slow_resp 1] 3] - - r config set commandlog-request-larger-than 0 - r commandlog reset large-request - r ping - set large_request_resp [r commandlog get -1 large-request] - assert_equal 2 [llength $large_request_resp] - assert_equal {ping} [lindex [lindex $large_request_resp 0] 3] - assert_equal {commandlog reset large-request} [lindex [lindex $large_request_resp 1] 3] - - r config set commandlog-reply-larger-than 0 - r commandlog reset large-reply - r ping - set large_reply_resp [r commandlog get -1 large-reply] - assert_equal 2 [llength $large_reply_resp] - assert_equal {ping} [lindex [lindex $large_reply_resp 0] 3] - assert_equal {commandlog reset large-reply} [lindex [lindex $large_reply_resp 1] 3] - } - - test {COMMANDLOG - config set / config get} { - r config set commandlog-request-larger-than 0 - r config set commandlog-reply-larger-than 0 - assert_equal [r config get commandlog-request-larger-than] {commandlog-request-larger-than 0} - assert_equal [r config get commandlog-reply-larger-than] {commandlog-reply-larger-than 0} - - r config set commandlog-request-larger-than -1 - r config set commandlog-reply-larger-than -1 - assert_equal [r config get commandlog-request-larger-than] {commandlog-request-larger-than -1} - assert_equal [r config get commandlog-reply-larger-than] {commandlog-reply-larger-than -1} - - r config set commandlog-request-larger-than 10MB - r config set commandlog-reply-larger-than 10MB - assert_equal [r config get commandlog-request-larger-than] {commandlog-request-larger-than 10485760} - assert_equal [r config get commandlog-reply-larger-than] {commandlog-reply-larger-than 10485760} - - assert_error {ERR *} {r config set commandlog-request-larger-than -100} - assert_error {ERR *} {r config set commandlog-reply-larger-than -100} - } } From 8e802cf75784fdad795e00d4f7015003404b70f5 Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 9 Apr 2026 11:18:28 +0800 Subject: [PATCH 7/8] Make commandlog config become signed memory config Signed-off-by: Binbin --- src/config.c | 4 ++-- tests/unit/commandlog.tcl | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/config.c b/src/config.c index 8b5c84ea899..f49e71b22ad 100644 --- a/src/config.c +++ b/src/config.c @@ -3437,8 +3437,8 @@ standardConfig static_configs[] = { createLongLongConfig("cluster-node-timeout", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.cluster_node_timeout, 15000, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("cluster-ping-interval", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, 0, LLONG_MAX, server.cluster_ping_interval, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("commandlog-execution-slower-than", "slowlog-log-slower-than", MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_SLOW].threshold, 10000, INTEGER_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, INTEGER_CONFIG, NULL, NULL), - createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, INTEGER_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-request-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REQUEST].threshold, 1024 * 1024, MEMORY_CONFIG | SIGNED_MEMORY_CONFIG, NULL, NULL), + createLongLongConfig("commandlog-reply-larger-than", NULL, MODIFIABLE_CONFIG, -1, LLONG_MAX, server.commandlog[COMMANDLOG_TYPE_LARGE_REPLY].threshold, 1024 * 1024, MEMORY_CONFIG | SIGNED_MEMORY_CONFIG, NULL, NULL), createLongLongConfig("latency-monitor-threshold", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.latency_monitor_threshold, 0, INTEGER_CONFIG, NULL, NULL), createLongLongConfig("proto-max-bulk-len", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.proto_max_bulk_len, 512ll * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Bulk request max size */ createLongLongConfig("stream-node-max-entries", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, server.stream_node_max_entries, 100, INTEGER_CONFIG, NULL, NULL), diff --git a/tests/unit/commandlog.tcl b/tests/unit/commandlog.tcl index 5fda2f0e111..a2c755c6611 100644 --- a/tests/unit/commandlog.tcl +++ b/tests/unit/commandlog.tcl @@ -408,4 +408,40 @@ start_server {tags {"commandlog"} overrides {commandlog-execution-slower-than 10 r config set min-string-size-avoid-copy-reply $copy_avoid r del testkey } + + test {COMMANDLOG - memory config with set / get / rewrite} { + r config set commandlog-request-larger-than 10mb + r config set commandlog-reply-larger-than 10mb + assert_equal [r config get commandlog-request-larger-than] {commandlog-request-larger-than 10485760} + assert_equal [r config get commandlog-reply-larger-than] {commandlog-reply-larger-than 10485760} + + r config rewrite + restart_server 0 true false + assert_equal [lindex [r config get commandlog-request-larger-than] 1] 10485760 + assert_equal [lindex [r config get commandlog-reply-larger-than] 1] 10485760 + } + + test {COMMANDLOG - special number -1 disables the command logging} { + r config set commandlog-execution-slower-than -1 + r config set commandlog-request-larger-than -1 + r config set commandlog-reply-larger-than -1 + assert_error {*argument must be between -1 and *} {r config set commandlog-execution-slower-than -2} + assert_error {*argument must be between -1 and *} {r config set commandlog-request-larger-than -2} + assert_error {*argument must be between -1 and *} {r config set commandlog-reply-larger-than -2} + + r commandlog reset slow + r commandlog reset large-request + r commandlog reset large-reply + + r ping + assert_equal [r commandlog len slow] 0 + assert_equal [r commandlog len large-request] 0 + assert_equal [r commandlog len large-reply] 0 + + r config rewrite + restart_server 0 true false + assert_equal [lindex [r config get commandlog-execution-slower-than] 1] -1 + assert_equal [lindex [r config get commandlog-request-larger-than] 1] -1 + assert_equal [lindex [r config get commandlog-reply-larger-than] 1] -1 + } } From 60ef7acb5395bfba62b051247bc49f16d31de75f Mon Sep 17 00:00:00 2001 From: Binbin Date: Fri, 10 Apr 2026 10:37:50 +0800 Subject: [PATCH 8/8] Add external:skip flag Signed-off-by: Binbin --- tests/unit/commandlog.tcl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/commandlog.tcl b/tests/unit/commandlog.tcl index a2c755c6611..4db492c3d13 100644 --- a/tests/unit/commandlog.tcl +++ b/tests/unit/commandlog.tcl @@ -419,7 +419,7 @@ start_server {tags {"commandlog"} overrides {commandlog-execution-slower-than 10 restart_server 0 true false assert_equal [lindex [r config get commandlog-request-larger-than] 1] 10485760 assert_equal [lindex [r config get commandlog-reply-larger-than] 1] 10485760 - } + } {} {external:skip} test {COMMANDLOG - special number -1 disables the command logging} { r config set commandlog-execution-slower-than -1 @@ -443,5 +443,5 @@ start_server {tags {"commandlog"} overrides {commandlog-execution-slower-than 10 assert_equal [lindex [r config get commandlog-execution-slower-than] 1] -1 assert_equal [lindex [r config get commandlog-request-larger-than] 1] -1 assert_equal [lindex [r config get commandlog-reply-larger-than] 1] -1 - } + } {} {external:skip} }