From 2fda5f5c98ca0f0ce426c7dff3951804db62e0fe Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Mon, 3 Feb 2020 15:19:44 +0530 Subject: [PATCH 01/21] Exclude "keymiss" notification from NOTIFY_ALL Because "keymiss" is "special" compared to the rest of the notifications (Trying not to break existing apps using the 'A' format for notifications) Also updated redis.conf and module.c docs --- redis.conf | 6 +++++- src/module.c | 3 ++- src/notify.c | 2 +- src/redismodule.h | 4 ++-- src/server.h | 4 ++-- 5 files changed, 12 insertions(+), 7 deletions(-) diff --git a/redis.conf b/redis.conf index 07005cffe..2b8711eea 100644 --- a/redis.conf +++ b/redis.conf @@ -1362,7 +1362,11 @@ latency-monitor-threshold 0 # z Sorted set commands # x Expired events (events generated every time a key expires) # e Evicted events (events generated when a key is evicted for maxmemory) -# A Alias for g$lshzxe, so that the "AKE" string means all the events. +# t Stream commands +# m Key-miss events (Note: It is not included in the 'A' class) +# A Alias for g$lshzxet, so that the "AKE" string means all the events +# (Except key-miss events which are excluded from 'A' due to their +# unique nature). # # The "notify-keyspace-events" takes as argument a string that is composed # of zero or multiple characters. The empty string means that notifications diff --git a/src/module.c b/src/module.c index d2b267be2..54072af57 100644 --- a/src/module.c +++ b/src/module.c @@ -4798,7 +4798,8 @@ void moduleReleaseGIL(void) { * - REDISMODULE_NOTIFY_EXPIRED: Expiration events * - REDISMODULE_NOTIFY_EVICTED: Eviction events * - REDISMODULE_NOTIFY_STREAM: Stream events - * - REDISMODULE_NOTIFY_ALL: All events + * - REDISMODULE_NOTIFY_KEYMISS: Key-miss events + * - REDISMODULE_NOTIFY_ALL: All events (Excluding REDISMODULE_NOTIFY_KEYMISS) * * We do not distinguish between key events and keyspace events, and it is up * to the module to filter the actions taken based on the key. diff --git a/src/notify.c b/src/notify.c index d6c3ad403..bb1055724 100644 --- a/src/notify.c +++ b/src/notify.c @@ -82,10 +82,10 @@ sds keyspaceEventsFlagsToString(int flags) { if (flags & NOTIFY_EXPIRED) res = sdscatlen(res,"x",1); if (flags & NOTIFY_EVICTED) res = sdscatlen(res,"e",1); if (flags & NOTIFY_STREAM) res = sdscatlen(res,"t",1); - if (flags & NOTIFY_KEY_MISS) res = sdscatlen(res,"m",1); } if (flags & NOTIFY_KEYSPACE) res = sdscatlen(res,"K",1); if (flags & NOTIFY_KEYEVENT) res = sdscatlen(res,"E",1); + if (flags & NOTIFY_KEY_MISS) res = sdscatlen(res,"m",1); return res; } diff --git a/src/redismodule.h b/src/redismodule.h index 637078f2b..e74611f13 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -127,8 +127,8 @@ #define REDISMODULE_NOTIFY_EXPIRED (1<<8) /* x */ #define REDISMODULE_NOTIFY_EVICTED (1<<9) /* e */ #define REDISMODULE_NOTIFY_STREAM (1<<10) /* t */ -#define REDISMODULE_NOTIFY_KEY_MISS (1<<11) /* m */ -#define REDISMODULE_NOTIFY_ALL (REDISMODULE_NOTIFY_GENERIC | REDISMODULE_NOTIFY_STRING | REDISMODULE_NOTIFY_LIST | REDISMODULE_NOTIFY_SET | REDISMODULE_NOTIFY_HASH | REDISMODULE_NOTIFY_ZSET | REDISMODULE_NOTIFY_EXPIRED | REDISMODULE_NOTIFY_EVICTED | REDISMODULE_NOTIFY_STREAM | REDISMODULE_NOTIFY_KEY_MISS) /* A */ +#define REDISMODULE_NOTIFY_KEY_MISS (1<<11) /* m (Note: This one is excluded from REDISMODULE_NOTIFY_ALL on purpose) */ +#define REDISMODULE_NOTIFY_ALL (REDISMODULE_NOTIFY_GENERIC | REDISMODULE_NOTIFY_STRING | REDISMODULE_NOTIFY_LIST | REDISMODULE_NOTIFY_SET | REDISMODULE_NOTIFY_HASH | REDISMODULE_NOTIFY_ZSET | REDISMODULE_NOTIFY_EXPIRED | REDISMODULE_NOTIFY_EVICTED | REDISMODULE_NOTIFY_STREAM) /* A */ /* A special pointer that we can use between the core and the module to signal diff --git a/src/server.h b/src/server.h index 8e354c03d..257f22985 100644 --- a/src/server.h +++ b/src/server.h @@ -413,8 +413,8 @@ typedef long long ustime_t; /* microsecond time type. */ #define NOTIFY_EXPIRED (1<<8) /* x */ #define NOTIFY_EVICTED (1<<9) /* e */ #define NOTIFY_STREAM (1<<10) /* t */ -#define NOTIFY_KEY_MISS (1<<11) /* m */ -#define NOTIFY_ALL (NOTIFY_GENERIC | NOTIFY_STRING | NOTIFY_LIST | NOTIFY_SET | NOTIFY_HASH | NOTIFY_ZSET | NOTIFY_EXPIRED | NOTIFY_EVICTED | NOTIFY_STREAM | NOTIFY_KEY_MISS) /* A flag */ +#define NOTIFY_KEY_MISS (1<<11) /* m (Note: This one is excluded from NOTIFY_ALL on purpose) */ +#define NOTIFY_ALL (NOTIFY_GENERIC | NOTIFY_STRING | NOTIFY_LIST | NOTIFY_SET | NOTIFY_HASH | NOTIFY_ZSET | NOTIFY_EXPIRED | NOTIFY_EVICTED | NOTIFY_STREAM) /* A flag */ /* Get the first bind addr or NULL */ #define NET_FIRST_BIND_ADDR (server.bindaddr_count ? server.bindaddr[0] : NULL) From 540b917a262c376f187dda51b4a7988aadef7ef1 Mon Sep 17 00:00:00 2001 From: lifubang Date: Tue, 4 Feb 2020 17:32:30 +0800 Subject: [PATCH 02/21] fix ssl flag check for redis-cli Signed-off-by: lifubang --- src/redis-cli.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/redis-cli.c b/src/redis-cli.c index 1919829e1..c1bda0a00 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -1595,15 +1595,15 @@ static int parseOptions(int argc, char **argv) { #ifdef USE_OPENSSL } else if (!strcmp(argv[i],"--tls")) { config.tls = 1; - } else if (!strcmp(argv[i],"--sni")) { + } else if (!strcmp(argv[i],"--sni") && !lastarg) { config.sni = argv[++i]; - } else if (!strcmp(argv[i],"--cacertdir")) { + } else if (!strcmp(argv[i],"--cacertdir") && !lastarg) { config.cacertdir = argv[++i]; - } else if (!strcmp(argv[i],"--cacert")) { + } else if (!strcmp(argv[i],"--cacert") && !lastarg) { config.cacert = argv[++i]; - } else if (!strcmp(argv[i],"--cert")) { + } else if (!strcmp(argv[i],"--cert") && !lastarg) { config.cert = argv[++i]; - } else if (!strcmp(argv[i],"--key")) { + } else if (!strcmp(argv[i],"--key") && !lastarg) { config.key = argv[++i]; #endif } else if (!strcmp(argv[i],"-v") || !strcmp(argv[i], "--version")) { @@ -1701,12 +1701,13 @@ static void usage(void) { " -c Enable cluster mode (follow -ASK and -MOVED redirections).\n" #ifdef USE_OPENSSL " --tls Establish a secure TLS connection.\n" -" --cacert CA Certificate file to verify with.\n" -" --cacertdir Directory where trusted CA certificates are stored.\n" +" --sni Server name indication for TLS.\n" +" --cacert CA Certificate file to verify with.\n" +" --cacertdir Directory where trusted CA certificates are stored.\n" " If neither cacert nor cacertdir are specified, the default\n" " system-wide trusted root certs configuration will apply.\n" -" --cert Client certificate to authenticate with.\n" -" --key Private key file to authenticate with.\n" +" --cert Client certificate to authenticate with.\n" +" --key Private key file to authenticate with.\n" #endif " --raw Use raw formatting for replies (default when STDOUT is\n" " not a tty).\n" From 9ac6cb9ce4095efc61cc1fd3786f0fbe01cfef37 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 5 Feb 2020 09:42:49 +0200 Subject: [PATCH 03/21] memoryGetKeys helper function so that ACL can limit access to keys for MEMORY command --- src/db.c | 16 ++++++++++++++++ src/server.c | 2 +- src/server.h | 1 + 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/db.c b/src/db.c index ba7be2725..88342ac4d 100644 --- a/src/db.c +++ b/src/db.c @@ -1522,6 +1522,22 @@ int *georadiusGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numk return keys; } +/* Helper function to extract keys from memory command. + * MEMORY USAGE */ +int *memoryGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys) { + int *keys; + UNUSED(cmd); + + if (argc >= 3 && !strcasecmp(argv[1]->ptr,"usage")) { + keys = zmalloc(sizeof(int) * 1); + keys[0] = 2; + *numkeys = 1; + return keys; + } + *numkeys = 0; + return NULL; +} + /* XREAD [BLOCK ] [COUNT ] [GROUP ] * STREAMS key_1 key_2 ... key_N ID_1 ID_2 ... ID_N */ int *xreadGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys) { diff --git a/src/server.c b/src/server.c index 2b226f568..f3b7c37c5 100644 --- a/src/server.c +++ b/src/server.c @@ -817,7 +817,7 @@ struct redisCommand redisCommandTable[] = { {"memory",memoryCommand,-2, "random read-only", - 0,NULL,0,0,0,0,0,0}, + 0,memoryGetKeys,0,0,0,0,0,0}, {"client",clientCommand,-2, "admin no-script random @connection", diff --git a/src/server.h b/src/server.h index 5d63e9b55..9a403a946 100644 --- a/src/server.h +++ b/src/server.h @@ -2074,6 +2074,7 @@ int *sortGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys); int *migrateGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys); int *georadiusGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys); int *xreadGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys); +int *memoryGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys); /* Cluster */ void clusterInit(void); From ffdd0620d0d6804b5628913b96392e0ea2e47e45 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 5 Feb 2020 11:41:24 +0200 Subject: [PATCH 04/21] config.c verbose error replies for CONFIG SET, like config file parsing We noticed that the error replies for the generic mechanism for enums are very verbose for config file parsing, but not for config set command. instead of replicating this code, i did a small refactoring to share code between CONFIG SET and config file parsing. and also renamed the enum group functions to be consistent with the naming of other types. --- src/config.c | 128 +++++++++++++-------------------------------------- 1 file changed, 31 insertions(+), 97 deletions(-) diff --git a/src/config.c b/src/config.c index 0526de84d..b19e78f74 100644 --- a/src/config.c +++ b/src/config.c @@ -190,8 +190,9 @@ typedef struct typeInterface { void (*init)(typeData data); /* Called on server start, should return 1 on success, 0 on error and should set err */ int (*load)(typeData data, sds *argc, int argv, char **err); - /* Called on CONFIG SET, returns 1 on success, 0 on error */ - int (*set)(typeData data, sds value, char **err); + /* Called on server startup and CONFIG SET, returns 1 on success, 0 on error + * and can set a verbose err string, update is true when called from CONFIG SET */ + int (*set)(typeData data, sds value, int update, char **err); /* Called on CONFIG GET, required to add output to the client */ void (*get)(client *c, typeData data); /* Called on CONFIG REWRITE, required to rewrite the config state */ @@ -323,7 +324,11 @@ void loadServerConfigFromString(char *config) { if ((!strcasecmp(argv[0],config->name) || (config->alias && !strcasecmp(argv[0],config->alias)))) { - if (!config->interface.load(config->data, argv, argc, &err)) { + if (argc != 2) { + err = "wrong number of arguments"; + goto loaderr; + } + if (!config->interface.set(config->data, argv[1], 0, &err)) { goto loaderr; } @@ -599,7 +604,7 @@ void configSetCommand(client *c) { if(config->modifiable && (!strcasecmp(c->argv[2]->ptr,config->name) || (config->alias && !strcasecmp(c->argv[2]->ptr,config->alias)))) { - if (!config->interface.set(config->data,o->ptr, &errstr)) { + if (!config->interface.set(config->data,o->ptr,1,&errstr)) { goto badfmt; } addReply(c,shared.ok); @@ -1536,9 +1541,8 @@ static char loadbuf[LOADBUF_SIZE]; .alias = (config_alias), \ .modifiable = (is_modifiable), -#define embedConfigInterface(initfn, loadfn, setfn, getfn, rewritefn) .interface = { \ +#define embedConfigInterface(initfn, setfn, getfn, rewritefn) .interface = { \ .init = (initfn), \ - .load = (loadfn), \ .set = (setfn), \ .get = (getfn), \ .rewrite = (rewritefn) \ @@ -1561,30 +1565,17 @@ static void boolConfigInit(typeData data) { *data.yesno.config = data.yesno.default_value; } -static int boolConfigLoad(typeData data, sds *argv, int argc, char **err) { - int yn; - if (argc != 2) { - *err = "wrong number of arguments"; - return 0; - } - if ((yn = yesnotoi(argv[1])) == -1) { +static int boolConfigSet(typeData data, sds value, int update, char **err) { + int yn = yesnotoi(value); + if (yn == -1) { *err = "argument must be 'yes' or 'no'"; return 0; } - if (data.yesno.is_valid_fn && !data.yesno.is_valid_fn(yn, err)) - return 0; - *data.yesno.config = yn; - return 1; -} - -static int boolConfigSet(typeData data, sds value, char **err) { - int yn = yesnotoi(value); - if (yn == -1) return 0; if (data.yesno.is_valid_fn && !data.yesno.is_valid_fn(yn, err)) return 0; int prev = *(data.yesno.config); *(data.yesno.config) = yn; - if (data.yesno.update_fn && !data.yesno.update_fn(yn, prev, err)) { + if (update && data.yesno.update_fn && !data.yesno.update_fn(yn, prev, err)) { *(data.yesno.config) = prev; return 0; } @@ -1601,7 +1592,7 @@ static void boolConfigRewrite(typeData data, const char *name, struct rewriteCon #define createBoolConfig(name, alias, modifiable, config_addr, default, is_valid, update) { \ embedCommonConfig(name, alias, modifiable) \ - embedConfigInterface(boolConfigInit, boolConfigLoad, boolConfigSet, boolConfigGet, boolConfigRewrite) \ + embedConfigInterface(boolConfigInit, boolConfigSet, boolConfigGet, boolConfigRewrite) \ .data.yesno = { \ .config = &(config_addr), \ .default_value = (default), \ @@ -1619,23 +1610,7 @@ static void stringConfigInit(typeData data) { } } -static int stringConfigLoad(typeData data, sds *argv, int argc, char **err) { - if (argc != 2) { - *err = "wrong number of arguments"; - return 0; - } - if (data.string.is_valid_fn && !data.string.is_valid_fn(argv[1], err)) - return 0; - zfree(*data.string.config); - if (data.string.convert_empty_to_null) { - *data.string.config = argv[1][0] ? zstrdup(argv[1]) : NULL; - } else { - *data.string.config = zstrdup(argv[1]); - } - return 1; -} - -static int stringConfigSet(typeData data, sds value, char **err) { +static int stringConfigSet(typeData data, sds value, int update, char **err) { if (data.string.is_valid_fn && !data.string.is_valid_fn(value, err)) return 0; char *prev = *data.string.config; @@ -1644,7 +1619,7 @@ static int stringConfigSet(typeData data, sds value, char **err) { } else { *data.string.config = zstrdup(value); } - if (data.string.update_fn && !data.string.update_fn(*data.string.config, prev, err)) { + if (update && data.string.update_fn && !data.string.update_fn(*data.string.config, prev, err)) { zfree(*data.string.config); *data.string.config = prev; return 0; @@ -1666,7 +1641,7 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC #define createStringConfig(name, alias, modifiable, empty_to_null, config_addr, default, is_valid, update) { \ embedCommonConfig(name, alias, modifiable) \ - embedConfigInterface(stringConfigInit, stringConfigLoad, stringConfigSet, stringConfigGet, stringConfigRewrite) \ + embedConfigInterface(stringConfigInit, stringConfigSet, stringConfigGet, stringConfigRewrite) \ .data.string = { \ .config = &(config_addr), \ .default_value = (default), \ @@ -1677,17 +1652,12 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC } /* Enum configs */ -static void configEnumInit(typeData data) { +static void enumConfigInit(typeData data) { *data.enumd.config = data.enumd.default_value; } -static int configEnumLoad(typeData data, sds *argv, int argc, char **err) { - if (argc != 2) { - *err = "wrong number of arguments"; - return 0; - } - - int enumval = configEnumGetValue(data.enumd.enum_value, argv[1]); +static int enumConfigSet(typeData data, sds value, int update, char **err) { + int enumval = configEnumGetValue(data.enumd.enum_value, value); if (enumval == INT_MIN) { sds enumerr = sdsnew("argument must be one of the following: "); configEnum *enumNode = data.enumd.enum_value; @@ -1707,37 +1677,28 @@ static int configEnumLoad(typeData data, sds *argv, int argc, char **err) { *err = loadbuf; return 0; } - if (data.enumd.is_valid_fn && !data.enumd.is_valid_fn(enumval, err)) - return 0; - *(data.enumd.config) = enumval; - return 1; -} - -static int configEnumSet(typeData data, sds value, char **err) { - int enumval = configEnumGetValue(data.enumd.enum_value, value); - if (enumval == INT_MIN) return 0; if (data.enumd.is_valid_fn && !data.enumd.is_valid_fn(enumval, err)) return 0; int prev = *(data.enumd.config); *(data.enumd.config) = enumval; - if (data.enumd.update_fn && !data.enumd.update_fn(enumval, prev, err)) { + if (update && data.enumd.update_fn && !data.enumd.update_fn(enumval, prev, err)) { *(data.enumd.config) = prev; return 0; } return 1; } -static void configEnumGet(client *c, typeData data) { +static void enumConfigGet(client *c, typeData data) { addReplyBulkCString(c, configEnumGetNameOrUnknown(data.enumd.enum_value,*data.enumd.config)); } -static void configEnumRewrite(typeData data, const char *name, struct rewriteConfigState *state) { +static void enumConfigRewrite(typeData data, const char *name, struct rewriteConfigState *state) { rewriteConfigEnumOption(state, name,*(data.enumd.config), data.enumd.enum_value, data.enumd.default_value); } #define createEnumConfig(name, alias, modifiable, enum, config_addr, default, is_valid, update) { \ embedCommonConfig(name, alias, modifiable) \ - embedConfigInterface(configEnumInit, configEnumLoad, configEnumSet, configEnumGet, configEnumRewrite) \ + embedConfigInterface(enumConfigInit, enumConfigSet, enumConfigGet, enumConfigRewrite) \ .data.enumd = { \ .config = &(config_addr), \ .default_value = (default), \ @@ -1832,49 +1793,22 @@ static int numericBoundaryCheck(typeData data, long long ll, char **err) { return 1; } -static int numericConfigLoad(typeData data, sds *argv, int argc, char **err) { - long long ll; - - if (argc != 2) { - *err = "wrong number of arguments"; - return 0; - } - +static int numericConfigSet(typeData data, sds value, int update, char **err) { + long long ll, prev = 0; if (data.numeric.is_memory) { int memerr; - ll = memtoll(argv[1], &memerr); + ll = memtoll(value, &memerr); if (memerr || ll < 0) { *err = "argument must be a memory value"; return 0; } } else { - if (!string2ll(argv[1], sdslen(argv[1]),&ll)) { + if (!string2ll(value, sdslen(value),&ll)) { *err = "argument couldn't be parsed into an integer" ; return 0; } } - if (!numericBoundaryCheck(data, ll, err)) - return 0; - - if (data.numeric.is_valid_fn && !data.numeric.is_valid_fn(ll, err)) - return 0; - - SET_NUMERIC_TYPE(ll) - - return 1; -} - -static int numericConfigSet(typeData data, sds value, char **err) { - long long ll, prev = 0; - if (data.numeric.is_memory) { - int memerr; - ll = memtoll(value, &memerr); - if (memerr || ll < 0) return 0; - } else { - if (!string2ll(value, sdslen(value),&ll)) return 0; - } - if (!numericBoundaryCheck(data, ll, err)) return 0; @@ -1884,7 +1818,7 @@ static int numericConfigSet(typeData data, sds value, char **err) { GET_NUMERIC_TYPE(prev) SET_NUMERIC_TYPE(ll) - if (data.numeric.update_fn && !data.numeric.update_fn(ll, prev, err)) { + if (update && data.numeric.update_fn && !data.numeric.update_fn(ll, prev, err)) { SET_NUMERIC_TYPE(prev) return 0; } @@ -1918,7 +1852,7 @@ static void numericConfigRewrite(typeData data, const char *name, struct rewrite #define embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) { \ embedCommonConfig(name, alias, modifiable) \ - embedConfigInterface(numericConfigInit, numericConfigLoad, numericConfigSet, numericConfigGet, numericConfigRewrite) \ + embedConfigInterface(numericConfigInit, numericConfigSet, numericConfigGet, numericConfigRewrite) \ .data.numeric = { \ .lower_bound = (lower), \ .upper_bound = (upper), \ From 3795aaf42aadf46b415edaa801383cd81085eb91 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 5 Feb 2020 18:15:38 +0200 Subject: [PATCH 05/21] update RM_SignalModifiedKey doc comment --- src/module.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/module.c b/src/module.c index 914c50df3..97edb97b3 100644 --- a/src/module.c +++ b/src/module.c @@ -890,7 +890,8 @@ void RM_SetModuleOptions(RedisModuleCtx *ctx, int options) { ctx->module->options = options; } -/* Signals that the key is modified from user's perspective (i.e. invalidate WATCH). */ +/* Signals that the key is modified from user's perspective (i.e. invalidate WATCH + * and client side caching). */ int RM_SignalModifiedKey(RedisModuleCtx *ctx, RedisModuleString *keyname) { signalModifiedKey(ctx->client->db,keyname); return REDISMODULE_OK; From 1e02d599dc2a0643fcf82af42047adf07c78fe41 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Wed, 5 Feb 2020 18:30:12 +0200 Subject: [PATCH 06/21] TLS: Some redis.conf clarifications. --- redis.conf | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/redis.conf b/redis.conf index 07005cffe..3c7336747 100644 --- a/redis.conf +++ b/redis.conf @@ -155,23 +155,22 @@ tcp-keepalive 300 # tls-ca-cert-file ca.crt # tls-ca-cert-dir /etc/ssl/certs -# If TLS/SSL clients are required to authenticate using a client side -# certificate, use this directive. +# By default, clients (including replica servers) on a TLS port are required +# to authenticate using valid client side certificates. # -# Note: this applies to all incoming clients, including replicas. +# It is possible to disable authentication using this directive. # -# tls-auth-clients yes +# tls-auth-clients no -# If TLS/SSL should be used when connecting as a replica to a master, enable -# this configuration directive: +# By default, a Redis replica does not attempt to establish a TLS connection +# with its master. +# +# Use the following directive to enable TLS on replication links. # # tls-replication yes -# If TLS/SSL should be used for the Redis Cluster bus, enable this configuration -# directive. -# -# NOTE: If TLS/SSL is enabled for Cluster Bus, mutual authentication is always -# enforced. +# By default, the Redis Cluster bus uses a plain TCP connection. To enable +# TLS for the bus protocol, use the following directive: # # tls-cluster yes From fe7e8dc955ef6c5f3b843f87763f62f6eb84bd4b Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 5 Feb 2020 19:47:09 +0200 Subject: [PATCH 07/21] Add handling of short read of module id in rdb --- src/rdb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/rdb.c b/src/rdb.c index 27e1b3135..61265433d 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -1871,7 +1871,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { } } else if (rdbtype == RDB_TYPE_MODULE || rdbtype == RDB_TYPE_MODULE_2) { uint64_t moduleid = rdbLoadLen(rdb,NULL); - if (rioGetReadError(rdb)) return NULL; + if (rioGetReadError(rdb)) { + rdbReportReadError("Short read module id"); + return NULL; + } moduleType *mt = moduleTypeLookupModuleByID(moduleid); char name[10]; From bb3d45a38683fc97c0b9b06ff7725fa1eca5d80c Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Wed, 5 Feb 2020 21:13:21 +0200 Subject: [PATCH 08/21] TLS: Update documentation. --- README.md | 18 ++++++++++++++++++ TLS.md | 45 ++++++++++++++------------------------------- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 3442659e6..c08013416 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,11 @@ It is as simple as: % make +To build with TLS support, you'll need OpenSSL development libraries (e.g. +libssl-dev on Debian/Ubuntu) and run: + + % make BUILD_TLS=yes + You can run a 32 bit Redis binary using: % make 32bit @@ -43,6 +48,13 @@ After building Redis, it is a good idea to test it using: % make test +If TLS is built, running the tests with TLS enabled (you will need `tcl-tls` +installed): + + % ./utils/gen-test-certs.sh + % ./runtest --tls + + Fixing build problems with dependencies or cached build options --------- @@ -125,6 +137,12 @@ as options using the command line. Examples: All the options in redis.conf are also supported as options using the command line, with exactly the same name. +Running Redis with TLS: +------------------ + +Please consult the [TLS.md](TLS.md) file for more information on +how to use Redis with TLS. + Playing with Redis ------------------ diff --git a/TLS.md b/TLS.md index 76fe0be2e..e480c1e9d 100644 --- a/TLS.md +++ b/TLS.md @@ -1,8 +1,5 @@ -TLS Support -- Work In Progress -=============================== - -This is a brief note to capture current thoughts/ideas and track pending action -items. +TLS Support +=========== Getting Started --------------- @@ -69,37 +66,23 @@ probably not be so hard. For cluster keys migration it might be more difficult, but there are probably other good reasons to improve that part anyway. To-Do List -========== +---------- -Additional TLS Features ------------------------ +- [ ] Add session caching support. Check if/how it's handled by clients to + assess how useful/important it is. +- [ ] redis-benchmark support. The current implementation is a mix of using + hiredis for parsing and basic networking (establishing connections), but + directly manipulating sockets for most actions. This will need to be cleaned + up for proper TLS support. The best approach is probably to migrate to hiredis + async mode. +- [ ] redis-cli `--slave` and `--rdb` support. -1. Add metrics to INFO? -2. Add session caching support. Check if/how it's handled by clients to assess - how useful/important it is. - -redis-benchmark ---------------- - -The current implementation is a mix of using hiredis for parsing and basic -networking (establishing connections), but directly manipulating sockets for -most actions. - -This will need to be cleaned up for proper TLS support. The best approach is -probably to migrate to hiredis async mode. - -redis-cli ---------- - -1. Add support for TLS in --slave and --rdb modes. - -Others ------- +Multi-port +---------- Consider the implications of allowing TLS to be configured on a separate port, -making Redis listening on multiple ports. +making Redis listening on multiple ports: -This impacts many things, like 1. Startup banner port notification 2. Proctitle 3. How slaves announce themselves From ac2c96f5b1714b36f215fc9caec87e746a804041 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 08:53:23 +0200 Subject: [PATCH 09/21] A few non-data commands that should be allowed while loading or stale SELECT, and HELLO are commands that may be executed by the client as soon as it connects, there's no reason to block them, preventing the client from doing the rest of his sequence (which might just be INFO or CONFIG, etc). MONITOR, DEBUG, SLOWLOG, TIME, LASTSAVE are all non-data accessing commands, which there's no reason to block. --- src/server.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/server.c b/src/server.c index f3b7c37c5..23e72ce19 100644 --- a/src/server.c +++ b/src/server.c @@ -579,7 +579,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"select",selectCommand,2, - "ok-loading fast @keyspace", + "ok-loading fast ok-stale @keyspace", 0,NULL,0,0,0,0,0,0}, {"swapdb",swapdbCommand,3, @@ -660,7 +660,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"lastsave",lastsaveCommand,1, - "read-only random fast @admin @dangerous", + "read-only random fast ok-loading ok-stale @admin @dangerous", 0,NULL,0,0,0,0,0,0}, {"type",typeCommand,2, @@ -708,7 +708,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"monitor",monitorCommand,1, - "admin no-script", + "admin no-script ok-loading ok-stale", 0,NULL,0,0,0,0,0,0}, {"ttl",ttlCommand,2, @@ -740,7 +740,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"debug",debugCommand,-2, - "admin no-script", + "admin no-script ok-loading ok-stale", 0,NULL,0,0,0,0,0,0}, {"config",configCommand,-2, @@ -820,11 +820,11 @@ struct redisCommand redisCommandTable[] = { 0,memoryGetKeys,0,0,0,0,0,0}, {"client",clientCommand,-2, - "admin no-script random @connection", + "admin no-script random ok-loading ok-stale @connection", 0,NULL,0,0,0,0,0,0}, {"hello",helloCommand,-2, - "no-auth no-script fast no-monitor no-slowlog @connection", + "no-auth no-script fast no-monitor ok-loading ok-stale no-slowlog @connection", 0,NULL,0,0,0,0,0,0}, /* EVAL can modify the dataset, however it is not flagged as a write @@ -838,7 +838,7 @@ struct redisCommand redisCommandTable[] = { 0,evalGetKeys,0,0,0,0,0,0}, {"slowlog",slowlogCommand,-2, - "admin random", + "admin random ok-loading ok-stale", 0,NULL,0,0,0,0,0,0}, {"script",scriptCommand,-2, @@ -846,7 +846,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"time",timeCommand,1, - "read-only random fast", + "read-only random fast ok-loading ok-stale", 0,NULL,0,0,0,0,0,0}, {"bitop",bitopCommand,-4, From 7e1d5954e59ab99c572f2dccccf6843c9aad9127 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 09:15:31 +0200 Subject: [PATCH 10/21] Memory leak when bind config is provided twice --- src/config.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/config.c b/src/config.c index b19e78f74..fe26b3884 100644 --- a/src/config.c +++ b/src/config.c @@ -349,6 +349,10 @@ void loadServerConfigFromString(char *config) { if (addresses > CONFIG_BINDADDR_MAX) { err = "Too many bind addresses specified"; goto loaderr; } + /* Free old bind addresses */ + for (j = 0; j < server.bindaddr_count; j++) { + zfree(server.bindaddr[j]); + } for (j = 0; j < addresses; j++) server.bindaddr[j] = zstrdup(argv[j+1]); server.bindaddr_count = addresses; From a17bddf2a14a1bd85a3606d2023fb9bb92004770 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 09:23:22 +0200 Subject: [PATCH 11/21] fix maxmemory config warning the warning condition was if usage > limit (saying it'll cause eviction or oom), but in fact the eviction and oom depends on used minus slave buffers. other than fixing the condition, i add info about the current usage and limit, which may be useful when looking at the log. --- src/config.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/config.c b/src/config.c index b19e78f74..173a90909 100644 --- a/src/config.c +++ b/src/config.c @@ -1995,8 +1995,9 @@ static int updateMaxmemory(long long val, long long prev, char **err) { UNUSED(prev); UNUSED(err); if (val) { - if ((unsigned long long)val < zmalloc_used_memory()) { - serverLog(LL_WARNING,"WARNING: the new maxmemory value set via CONFIG SET is smaller than the current memory usage. This will result in key eviction and/or the inability to accept new write commands depending on the maxmemory-policy."); + size_t used = zmalloc_used_memory()-freeMemoryGetNotCountedMemory(); + if ((unsigned long long)val < used) { + serverLog(LL_WARNING,"WARNING: the new maxmemory value set via CONFIG SET (%llu) is smaller than the current memory usage (%zu). This will result in key eviction and/or the inability to accept new write commands depending on the maxmemory-policy.", server.maxmemory, used); } freeMemoryIfNeededAndSafe(); } From d454d5a4f516991c0f1e21a285c71f38a113e297 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 09:33:20 +0200 Subject: [PATCH 12/21] Fix client flags to be int64 in module.c currently there's no bug since the flags these functions handle are always lower than 32bit, but still better fix the type to prevent future bugs. --- src/module.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/module.c b/src/module.c index 914c50df3..a2bc869f9 100644 --- a/src/module.c +++ b/src/module.c @@ -714,9 +714,9 @@ void RM_KeyAtPos(RedisModuleCtx *ctx, int pos) { * flags into the command flags used by the Redis core. * * It returns the set of flags, or -1 if unknown flags are found. */ -int commandFlagsFromString(char *s) { +int64_t commandFlagsFromString(char *s) { int count, j; - int flags = 0; + int64_t flags = 0; sds *tokens = sdssplitlen(s,strlen(s)," ",1,&count); for (j = 0; j < count; j++) { char *t = tokens[j]; @@ -798,7 +798,7 @@ int commandFlagsFromString(char *s) { * to authenticate a client. */ int RM_CreateCommand(RedisModuleCtx *ctx, const char *name, RedisModuleCmdFunc cmdfunc, const char *strflags, int firstkey, int lastkey, int keystep) { - int flags = strflags ? commandFlagsFromString((char*)strflags) : 0; + int64_t flags = strflags ? commandFlagsFromString((char*)strflags) : 0; if (flags == -1) return REDISMODULE_ERR; if ((flags & CMD_MODULE_NO_CLUSTER) && server.cluster_enabled) return REDISMODULE_ERR; From 85cc696f50064327e81a41952ffb2be830048701 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 09:37:04 +0200 Subject: [PATCH 13/21] moduleRDBLoadError, add key name, and use panic rather than exit using panic rather than exit means you get s stack trace of the code path that experianced the error, and possibly other info. --- src/module.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/module.c b/src/module.c index 914c50df3..16b191535 100644 --- a/src/module.c +++ b/src/module.c @@ -3649,14 +3649,15 @@ void moduleRDBLoadError(RedisModuleIO *io) { io->error = 1; return; } - serverLog(LL_WARNING, + serverPanic( "Error loading data from RDB (short read or EOF). " "Read performed by module '%s' about type '%s' " - "after reading '%llu' bytes of a value.", + "after reading '%llu' bytes of a value " + "for key named: '%s'.", io->type->module->name, io->type->name, - (unsigned long long)io->bytes); - exit(1); + (unsigned long long)io->bytes, + io->key? (char*)io->key->ptr: "(null)"); } /* Returns 0 if there's at least one registered data type that did not declare From 485d5d4a18441feff626e61e31e480c3054fe877 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 09:41:45 +0200 Subject: [PATCH 14/21] reduce repeated calls to use_diskless_load this function possibly iterates on the module list --- src/replication.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/replication.c b/src/replication.c index b7e77184a..7aebd620a 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1354,7 +1354,7 @@ void disklessLoadRestoreBackups(redisDb *backup, int restore, int empty_db_flags void readSyncBulkPayload(connection *conn) { char buf[4096]; ssize_t nread, readlen, nwritten; - int use_diskless_load; + int use_diskless_load = useDisklessLoad(); redisDb *diskless_load_backup = NULL; int empty_db_flags = server.repl_slave_lazy_flush ? EMPTYDB_ASYNC : EMPTYDB_NO_FLAGS; @@ -1411,19 +1411,18 @@ void readSyncBulkPayload(connection *conn) { server.repl_transfer_size = 0; serverLog(LL_NOTICE, "MASTER <-> REPLICA sync: receiving streamed RDB from master with EOF %s", - useDisklessLoad()? "to parser":"to disk"); + use_diskless_load? "to parser":"to disk"); } else { usemark = 0; server.repl_transfer_size = strtol(buf+1,NULL,10); serverLog(LL_NOTICE, "MASTER <-> REPLICA sync: receiving %lld bytes from master %s", (long long) server.repl_transfer_size, - useDisklessLoad()? "to parser":"to disk"); + use_diskless_load? "to parser":"to disk"); } return; } - use_diskless_load = useDisklessLoad(); if (!use_diskless_load) { /* Read the data from the socket, store it to a file and search * for the EOF. */ From 86e302f5f302da89790f03000e9f76d322b5f971 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 10:07:17 +0200 Subject: [PATCH 15/21] freeClientAsync don't lock mutex if there's just one thread --- src/networking.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index a2e454d4b..1c9325c43 100644 --- a/src/networking.c +++ b/src/networking.c @@ -1162,9 +1162,14 @@ void freeClientAsync(client *c) { * may access the list while Redis uses I/O threads. All the other accesses * are in the context of the main thread while the other threads are * idle. */ - static pthread_mutex_t async_free_queue_mutex = PTHREAD_MUTEX_INITIALIZER; if (c->flags & CLIENT_CLOSE_ASAP || c->flags & CLIENT_LUA) return; c->flags |= CLIENT_CLOSE_ASAP; + if (server.io_threads_num == 1) { + /* no need to bother with locking if there's just one thread (the main thread) */ + listAddNodeTail(server.clients_to_close,c); + return; + } + static pthread_mutex_t async_free_queue_mutex = PTHREAD_MUTEX_INITIALIZER; pthread_mutex_lock(&async_free_queue_mutex); listAddNodeTail(server.clients_to_close,c); pthread_mutex_unlock(&async_free_queue_mutex); From aac6a4cf135aa136b339e84c23db3cfc06b4a776 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 10:14:32 +0200 Subject: [PATCH 16/21] move restartAOFAfterSYNC from replicaofCommand to replicationUnsetMaster replicationUnsetMaster can be called from other places, not just replicaofCOmmand, and all of these need to restart AOF --- src/replication.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/replication.c b/src/replication.c index b7e77184a..5499ebc57 100644 --- a/src/replication.c +++ b/src/replication.c @@ -2399,6 +2399,10 @@ void replicationUnsetMaster(void) { moduleFireServerEvent(REDISMODULE_EVENT_REPLICATION_ROLE_CHANGED, REDISMODULE_EVENT_REPLROLECHANGED_NOW_MASTER, NULL); + + /* Restart the AOF subsystem in case we shut it down during a sync when + * we were still a slave. */ + if (server.aof_enabled && server.aof_state == AOF_OFF) restartAOFAfterSYNC(); } /* This function is called when the slave lose the connection with the @@ -2436,9 +2440,6 @@ void replicaofCommand(client *c) { serverLog(LL_NOTICE,"MASTER MODE enabled (user request from '%s')", client); sdsfree(client); - /* Restart the AOF subsystem in case we shut it down during a sync when - * we were still a slave. */ - if (server.aof_enabled && server.aof_state == AOF_OFF) restartAOFAfterSYNC(); } } else { long port; From 69e8ea7143061eda18c1aa1ca9a345831cf5f884 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 10:17:34 +0200 Subject: [PATCH 17/21] stopAppendOnly resets aof_rewrite_scheduled althouh in theory, users can do BGREWRITEAOF even if aof is disabled, i suppose it is more common that the scheduled flag is set by either startAppendOnly, of a failed initial AOFRW fork (AOF_WAIT_REWRITE) --- src/aof.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/aof.c b/src/aof.c index 9eeb3f1e2..3682c4568 100644 --- a/src/aof.c +++ b/src/aof.c @@ -242,6 +242,7 @@ void stopAppendOnly(void) { server.aof_fd = -1; server.aof_selected_db = -1; server.aof_state = AOF_OFF; + server.aof_rewrite_scheduled = 0; killAppendOnlyChild(); } From 6d29c34da7e0caeeb1647f41056f964e3a1ecc68 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 10:31:43 +0200 Subject: [PATCH 18/21] add SAVE subcommand to ACL HELP and top comment --- src/acl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/acl.c b/src/acl.c index 1f395bd3f..aae35226f 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1461,6 +1461,7 @@ void ACLLoadUsersAtStartup(void) { /* ACL -- show and modify the configuration of ACL users. * ACL HELP * ACL LOAD + * ACL SAVE * ACL LIST * ACL USERS * ACL CAT [] @@ -1658,6 +1659,7 @@ void aclCommand(client *c) { } else if (!strcasecmp(sub,"help")) { const char *help[] = { "LOAD -- Reload users from the ACL file.", +"SAVE -- Save the current config to the ACL file." "LIST -- Show user details in config file format.", "USERS -- List all the registered usernames.", "SETUSER [attribs ...] -- Create or modify a user.", From e33fffbde1623a83895a9aba80f2fdd25ec84330 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Thu, 6 Feb 2020 14:09:45 +0530 Subject: [PATCH 19/21] Fix small bugs related to replica and monitor ambiguity 1. server.repl_no_slaves_since can be set when a MONITOR client disconnects 2. c->repl_ack_time can be set by a newline from a MONITOR client 3. Improved comments --- src/networking.c | 12 +++++++----- src/server.c | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/networking.c b/src/networking.c index a2e454d4b..8fca3fc1d 100644 --- a/src/networking.c +++ b/src/networking.c @@ -1121,7 +1121,7 @@ void freeClient(client *c) { /* We need to remember the time when we started to have zero * attached slaves, as after some time we'll free the replication * backlog. */ - if (c->flags & CLIENT_SLAVE && listLength(server.slaves) == 0) + if (getClientType(c) == CLIENT_TYPE_SLAVE && listLength(server.slaves) == 0) server.repl_no_slaves_since = server.unixtime; refreshGoodSlavesCount(); /* Fire the replica change modules event. */ @@ -1252,8 +1252,8 @@ int writeToClient(client *c, int handler_installed) { * just deliver as much data as it is possible to deliver. * * Moreover, we also send as much as possible if the client is - * a slave (otherwise, on high-speed traffic, the replication - * buffer will grow indefinitely) */ + * a slave or a monitor (otherwise, on high-speed traffic, the + * replication/output buffer will grow indefinitely) */ if (totwritten > NET_MAX_WRITES_PER_EVENT && (server.maxmemory == 0 || zmalloc_used_memory() < server.maxmemory) && @@ -1439,7 +1439,7 @@ int processInlineBuffer(client *c) { /* Newline from slaves can be used to refresh the last ACK time. * This is useful for a slave to ping back while loading a big * RDB file. */ - if (querylen == 0 && c->flags & CLIENT_SLAVE) + if (querylen == 0 && getClientType(c) == CLIENT_TYPE_SLAVE) c->repl_ack_time = server.unixtime; /* Move querybuffer position to the next query in the buffer. */ @@ -2433,12 +2433,14 @@ unsigned long getClientOutputBufferMemoryUsage(client *c) { * * The function will return one of the following: * CLIENT_TYPE_NORMAL -> Normal client - * CLIENT_TYPE_SLAVE -> Slave or client executing MONITOR command + * CLIENT_TYPE_SLAVE -> Slave * CLIENT_TYPE_PUBSUB -> Client subscribed to Pub/Sub channels * CLIENT_TYPE_MASTER -> The client representing our replication master. */ int getClientType(client *c) { if (c->flags & CLIENT_MASTER) return CLIENT_TYPE_MASTER; + /* Even though MONITOR clients are marked as replicas, we + * want the expose them as normal clients. */ if ((c->flags & CLIENT_SLAVE) && !(c->flags & CLIENT_MONITOR)) return CLIENT_TYPE_SLAVE; if (c->flags & CLIENT_PUBSUB) return CLIENT_TYPE_PUBSUB; diff --git a/src/server.c b/src/server.c index f3b7c37c5..2b32df0db 100644 --- a/src/server.c +++ b/src/server.c @@ -1498,7 +1498,7 @@ int clientsCronHandleTimeout(client *c, mstime_t now_ms) { time_t now = now_ms/1000; if (server.maxidletime && - !(c->flags & CLIENT_SLAVE) && /* no timeout for slaves */ + !(c->flags & CLIENT_SLAVE) && /* no timeout for slaves and monitors */ !(c->flags & CLIENT_MASTER) && /* no timeout for masters */ !(c->flags & CLIENT_BLOCKED) && /* no timeout for BLPOP */ !(c->flags & CLIENT_PUBSUB) && /* no timeout for Pub/Sub clients */ From 31ffbf11332fbc6afd69ca94f77ea66bbfb918e3 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 10:40:29 +0200 Subject: [PATCH 20/21] DEBUG HELP - add PROTOCOL --- src/debug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/debug.c b/src/debug.c index a2d37337d..b910d2d2d 100644 --- a/src/debug.c +++ b/src/debug.c @@ -355,6 +355,7 @@ void debugCommand(client *c) { "CRASH-AND-RECOVER -- Hard crash and restart after delay.", "DIGEST -- Output a hex signature representing the current DB content.", "DIGEST-VALUE ... -- Output a hex signature of the values of all the specified keys.", +"DEBUG PROTOCOL [string|integer|double|bignum|null|array|set|map|attrib|push|verbatim|true|false]", "ERROR -- Return a Redis protocol error with as message. Useful for clients unit tests to simulate Redis errors.", "LOG -- write message to the server log.", "HTSTATS -- Return hash table statistics of the specified Redis database.", @@ -586,7 +587,7 @@ NULL } } else if (!strcasecmp(c->argv[1]->ptr,"protocol") && c->argc == 3) { /* DEBUG PROTOCOL [string|integer|double|bignum|null|array|set|map| - * attrib|push|verbatim|true|false|state|err|bloberr] */ + * attrib|push|verbatim|true|false] */ char *name = c->argv[2]->ptr; if (!strcasecmp(name,"string")) { addReplyBulkCString(c,"Hello World"); @@ -634,7 +635,7 @@ NULL } else if (!strcasecmp(name,"verbatim")) { addReplyVerbatim(c,"This is a verbatim\nstring",25,"txt"); } else { - addReplyError(c,"Wrong protocol type name. Please use one of the following: string|integer|double|bignum|null|array|set|map|attrib|push|verbatim|true|false|state|err|bloberr"); + addReplyError(c,"Wrong protocol type name. Please use one of the following: string|integer|double|bignum|null|array|set|map|attrib|push|verbatim|true|false"); } } else if (!strcasecmp(c->argv[1]->ptr,"sleep") && c->argc == 3) { double dtime = strtod(c->argv[2]->ptr,NULL); From 91c41b6ddee22eec7757a60fc87c24fd5087306a Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Thu, 6 Feb 2020 14:12:08 +0530 Subject: [PATCH 21/21] Some refactroing using getClientType instead of CLIENT_SLAVE --- src/networking.c | 9 +++++---- src/object.c | 35 +++++++++++++---------------------- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/src/networking.c b/src/networking.c index 8fca3fc1d..496b0f3dc 100644 --- a/src/networking.c +++ b/src/networking.c @@ -369,9 +369,10 @@ void addReplyErrorLength(client *c, const char *s, size_t len) { * Where the master must propagate the first change even if the second * will produce an error. However it is useful to log such events since * they are rare and may hint at errors in a script or a bug in Redis. */ - if (c->flags & (CLIENT_MASTER|CLIENT_SLAVE) && !(c->flags & CLIENT_MONITOR)) { - char* to = c->flags & CLIENT_MASTER? "master": "replica"; - char* from = c->flags & CLIENT_MASTER? "replica": "master"; + int ctype = getClientType(c); + if (ctype == CLIENT_TYPE_MASTER || ctype == CLIENT_TYPE_SLAVE) { + char* to = ctype == CLIENT_TYPE_MASTER? "master": "replica"; + char* from = ctype == CLIENT_TYPE_MASTER? "replica": "master"; char *cmdname = c->lastcmd ? c->lastcmd->name : ""; serverLog(LL_WARNING,"== CRITICAL == This %s is sending an error " "to its %s: '%s' after processing the command " @@ -1074,7 +1075,7 @@ void freeClient(client *c) { } /* Log link disconnection with slave */ - if ((c->flags & CLIENT_SLAVE) && !(c->flags & CLIENT_MONITOR)) { + if (getClientType(c) == CLIENT_TYPE_SLAVE) { serverLog(LL_WARNING,"Connection with replica %s lost.", replicationGetSlaveName(c)); } diff --git a/src/object.c b/src/object.c index 52007b474..cc6b218a0 100644 --- a/src/object.c +++ b/src/object.c @@ -974,38 +974,29 @@ struct redisMemOverhead *getMemoryOverheadData(void) { mh->repl_backlog = mem; mem_total += mem; - mem = 0; - if (listLength(server.slaves)) { - listIter li; - listNode *ln; - - listRewind(server.slaves,&li); - while((ln = listNext(&li))) { - client *c = listNodeValue(ln); - mem += getClientOutputBufferMemoryUsage(c); - mem += sdsAllocSize(c->querybuf); - mem += sizeof(client); - } - } - mh->clients_slaves = mem; - mem_total+=mem; - mem = 0; if (listLength(server.clients)) { listIter li; listNode *ln; + size_t mem_normal = 0, mem_slaves = 0; listRewind(server.clients,&li); while((ln = listNext(&li))) { + size_t mem_curr = 0; client *c = listNodeValue(ln); - if (c->flags & CLIENT_SLAVE && !(c->flags & CLIENT_MONITOR)) - continue; - mem += getClientOutputBufferMemoryUsage(c); - mem += sdsAllocSize(c->querybuf); - mem += sizeof(client); + int type = getClientType(c); + mem_curr += getClientOutputBufferMemoryUsage(c); + mem_curr += sdsAllocSize(c->querybuf); + mem_curr += sizeof(client); + if (type == CLIENT_TYPE_SLAVE) + mem_slaves += mem_curr; + else + mem_normal += mem_curr; } + mh->clients_slaves = mem_slaves; + mh->clients_normal = mem_normal; + mem = mem_slaves + mem_normal; } - mh->clients_normal = mem; mem_total+=mem; mem = 0;