From 5def65008ff92519a828e1ba403e9a46836ca802 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 21 Feb 2018 11:04:13 +0200 Subject: [PATCH 01/60] Fix zrealloc to behave similarly to je_realloc when size is 0 According to C11, the behavior of realloc with size 0 is now deprecated. it can either behave as free(ptr) and return NULL, or return a valid pointer. but in zmalloc it can lead to zmalloc_oom_handler and panic. and that can affect modules that use it. It looks like both glibc allocator and jemalloc behave like so: realloc(malloc(32),0) returns NULL realloc(NULL,0) returns a valid pointer This commit changes zmalloc to behave the same --- src/zmalloc.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/zmalloc.c b/src/zmalloc.c index 094dd80f..01ac8c79 100644 --- a/src/zmalloc.c +++ b/src/zmalloc.c @@ -147,6 +147,10 @@ void *zrealloc(void *ptr, size_t size) { size_t oldsize; void *newptr; + if (size == 0 && ptr!=NULL) { + zfree(ptr); + return NULL; + } if (ptr == NULL) return zmalloc(size); #ifdef HAVE_MALLOC_SIZE oldsize = zmalloc_size(ptr); From b660fc2fbe545f4a20a907ffa6c8333396435907 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Wed, 7 Mar 2018 10:40:37 +0700 Subject: [PATCH 02/60] Fix zlexrangespec mem-leak in genericZrangebylexCommand --- src/t_zset.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/t_zset.c b/src/t_zset.c index f7f4c6eb..fa7793b1 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -2856,7 +2856,10 @@ void genericZrangebylexCommand(client *c, int reverse) { while (remaining) { if (remaining >= 3 && !strcasecmp(c->argv[pos]->ptr,"limit")) { if ((getLongFromObjectOrReply(c, c->argv[pos+1], &offset, NULL) != C_OK) || - (getLongFromObjectOrReply(c, c->argv[pos+2], &limit, NULL) != C_OK)) return; + (getLongFromObjectOrReply(c, c->argv[pos+2], &limit, NULL) != C_OK)) { + zslFreeLexRange(&range); + return; + } pos += 3; remaining -= 3; } else { zslFreeLexRange(&range); From 8c8e85df874c852b5f125209e9d662a70e310f66 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Wed, 18 Apr 2018 13:01:53 +0300 Subject: [PATCH 03/60] Use memtoll() in 'CONFIG SET client-output-buffer-limit' --- src/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.c b/src/config.c index a1122d05..a33981c6 100644 --- a/src/config.c +++ b/src/config.c @@ -983,8 +983,8 @@ void configSetCommand(client *c) { int soft_seconds; class = getClientTypeByName(v[j]); - hard = strtoll(v[j+1],NULL,10); - soft = strtoll(v[j+2],NULL,10); + hard = memtoll(v[j+1],NULL); + soft = memtoll(v[j+2],NULL); soft_seconds = strtoll(v[j+3],NULL,10); server.client_obuf_limits[class].hard_limit_bytes = hard; From 17c5f17686354b28c715b6f16c9c4e8eb2239df4 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Mon, 13 Aug 2018 17:43:29 +0300 Subject: [PATCH 04/60] Add log when server dies of SIGTERM during loading this is very confusing to see the server disappears as if it got SIGKILL when it was not the case. --- src/server.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/server.c b/src/server.c index b537ee04..0c665033 100644 --- a/src/server.c +++ b/src/server.c @@ -3780,6 +3780,7 @@ static void sigShutdownHandler(int sig) { rdbRemoveTempFile(getpid()); exit(1); /* Exit with an error since this was not a clean shutdown. */ } else if (server.loading) { + serverLogFromHandler(LL_WARNING, "Received shutdown signal during loading, exiting now."); exit(0); } From ed88f77d6dcd36e0c62faa484491530bd6739d38 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Thu, 13 Dec 2018 13:57:38 +0100 Subject: [PATCH 05/60] Check server.verbosity in RM_LogRaw --- src/module.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/module.c b/src/module.c index 20d159d3..e553bc0d 100644 --- a/src/module.c +++ b/src/module.c @@ -3421,6 +3421,8 @@ void RM_LogRaw(RedisModule *module, const char *levelstr, const char *fmt, va_li else if (!strcasecmp(levelstr,"warning")) level = LL_WARNING; else level = LL_VERBOSE; /* Default. */ + if (level < server.verbosity) return; + name_len = snprintf(msg, sizeof(msg),"<%s> ", module->name); vsnprintf(msg + name_len, sizeof(msg) - name_len, fmt, ap); serverLogRaw(level,msg); From ab37289fa6035a774d7438f8a7342d3177fdc1be Mon Sep 17 00:00:00 2001 From: MeirShpilraien Date: Sun, 9 Dec 2018 14:32:55 +0200 Subject: [PATCH 06/60] added module ability to register api to be used by other modules --- src/module.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++ src/redismodule.h | 4 ++ src/server.h | 1 + 3 files changed, 120 insertions(+) diff --git a/src/module.c b/src/module.c index 20d159d3..d177af24 100644 --- a/src/module.c +++ b/src/module.c @@ -47,7 +47,16 @@ struct RedisModule { int ver; /* Module version. We use just progressive integers. */ int apiver; /* Module API version as requested during initialization.*/ list *types; /* Module data types. */ + list *usedBy; /* list of modules names using this module api. */ + list *using; /* list of modules names that this module is using thier api . */ + list *exportedFunctions; /* list of function names exported by this module. */ }; + +struct ModuleExportedApi { + void* funcPointer; + struct RedisModule* module; +}; + typedef struct RedisModule RedisModule; static dict *modules; /* Hash table of modules. SDS -> RedisModule ptr.*/ @@ -700,6 +709,9 @@ void RM_SetModuleAttribs(RedisModuleCtx *ctx, const char *name, int ver, int api module->ver = ver; module->apiver = apiver; module->types = listCreate(); + module->usedBy = listCreate(); + module->using = listCreate(); + module->exportedFunctions = listCreate(); ctx->module = module; } @@ -4615,6 +4627,59 @@ void RM_GetRandomHexChars(char *dst, size_t len) { getRandomHexChars(dst,len); } +/* Used to register an api to be used by other modules. */ +int RM_RegisterApi(RedisModuleCtx *ctx, const char *funcname, void *funcptr) { + struct ModuleExportedApi* eapi = zmalloc(sizeof(*eapi)); + eapi->module = ctx->module; + eapi->funcPointer = funcptr; + if(dictAdd(server.exportedapi, (char*)funcname, eapi) != DICT_OK){ + zfree(eapi); + return REDISMODULE_ERR; + } + listAddNodeHead(ctx->module->exportedFunctions, (char*)funcname); + return REDISMODULE_OK; +} + +static inline int IsModuleInList(list *l, const char* moduleName){ + listIter *iter = listGetIterator(l, AL_START_HEAD); + listNode *node = NULL; + while((node = listNext(iter))){ + char* name = listNodeValue(node); + if(strcmp(name, moduleName) == 0){ + listReleaseIterator(iter); + return 1; + } + } + listReleaseIterator(iter); + return 0; +} + +static inline void RemoveFromList(list *l, const char* moduleName){ + listIter *iter = listGetIterator(l, AL_START_HEAD); + listNode *node = NULL; + while((node = listNext(iter))){ + char* name = listNodeValue(node); + if(strcmp(name, moduleName) == 0){ + listDelNode(l, node); + return; + } + } + listReleaseIterator(iter); +} + +void* RM_GetExportedApi(RedisModuleCtx *ctx, const char *funcname) { + dictEntry* entry = dictFind(server.exportedapi, funcname); + if(!entry){ + return NULL; + } + struct ModuleExportedApi* eapi = dictGetVal(entry); + if(!IsModuleInList(eapi->module->usedBy, ctx->module->name)){ + listAddNodeHead(eapi->module->usedBy, ctx->module->name); + listAddNodeHead(ctx->module->using, eapi->module->name); + } + return eapi->funcPointer; +} + /* -------------------------------------------------------------------------- * Modules API internals * -------------------------------------------------------------------------- */ @@ -4735,6 +4800,28 @@ void moduleUnregisterCommands(struct RedisModule *module) { dictReleaseIterator(di); } +void moduleUnregisterApi(struct RedisModule *module) { + listIter *iter = listGetIterator(module->exportedFunctions, AL_START_HEAD); + listNode* node = NULL; + while((node = listNext(iter))){ + char* functionName = listNodeValue(node); + struct ModuleExportedApi* eapi = dictFetchValue(server.exportedapi, functionName); + serverAssert(eapi); + zfree(eapi); + dictDelete(server.exportedapi, functionName); + } + listReleaseIterator(iter); + iter = listGetIterator(module->using, AL_START_HEAD); + node = NULL; + while((node = listNext(iter))){ + char* moduleName = listNodeValue(node); + struct RedisModule* usingModule = dictFetchValue(modules, moduleName); + serverAssert(usingModule); + RemoveFromList(usingModule->usedBy, module->name); + } + listReleaseIterator(iter); +} + /* Load a module and initialize it. On success C_OK is returned, otherwise * C_ERR is returned. */ int moduleLoad(const char *path, void **module_argv, int module_argc) { @@ -4794,6 +4881,12 @@ int moduleUnload(sds name) { return REDISMODULE_ERR; } + if (listLength(module->usedBy)) { + errno = EPERM; + return REDISMODULE_ERR; + } + + moduleUnregisterApi(module); moduleUnregisterCommands(module); /* Remove any notification subscribers this module might have */ @@ -4826,6 +4919,7 @@ void moduleCommand(client *c) { if (c->argc == 2 && !strcasecmp(subcmd,"help")) { const char *help[] = { "LIST -- Return a list of loaded modules.", +"LISTAPI -- Return a list of exported api.", "LOAD [arg ...] -- Load a module library from .", "UNLOAD -- Unload a module.", NULL @@ -4858,6 +4952,9 @@ NULL case EBUSY: errmsg = "the module exports one or more module-side data types, can't unload"; break; + case EPERM: + errmsg = "the module api is used by other modules, please unload them first and try again."; + break; default: errmsg = "operation not possible."; break; @@ -4879,6 +4976,21 @@ NULL addReplyLongLong(c,module->ver); } dictReleaseIterator(di); + } else if (!strcasecmp(subcmd,"listapi") && c->argc == 3) { + char *moduleName = c->argv[2]->ptr; + struct RedisModule* module = dictFetchValue(modules, moduleName); + if(!module){ + addReplyErrorFormat(c,"Error listing module api: no such module %s", moduleName); + return; + } + addReplyMultiBulkLen(c,listLength(module->exportedFunctions)); + listIter *iter = listGetIterator(module->exportedFunctions, AL_START_HEAD); + listNode* node = NULL; + while((node = listNext(iter))){ + char* functionName = listNodeValue(node); + addReplyBulkCString(c,functionName); + } + listReleaseIterator(iter); } else { addReplySubcommandSyntaxError(c); return; @@ -4894,6 +5006,7 @@ size_t moduleCount(void) { * file so that's easy to seek it to add new entries. */ void moduleRegisterCoreAPI(void) { server.moduleapi = dictCreate(&moduleAPIDictType,NULL); + server.exportedapi = dictCreate(&moduleAPIDictType,NULL); REGISTER_API(Alloc); REGISTER_API(Calloc); REGISTER_API(Realloc); @@ -5044,4 +5157,6 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(DictPrev); REGISTER_API(DictCompareC); REGISTER_API(DictCompare); + REGISTER_API(RegisterApi); + REGISTER_API(GetExportedApi); } diff --git a/src/redismodule.h b/src/redismodule.h index d18c3888..3c76fa02 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -332,6 +332,8 @@ void REDISMODULE_API_FUNC(RedisModule_GetRandomBytes)(unsigned char *dst, size_t void REDISMODULE_API_FUNC(RedisModule_GetRandomHexChars)(char *dst, size_t len); void REDISMODULE_API_FUNC(RedisModule_SetDisconnectCallback)(RedisModuleBlockedClient *bc, RedisModuleDisconnectFunc callback); void REDISMODULE_API_FUNC(RedisModule_SetClusterFlags)(RedisModuleCtx *ctx, uint64_t flags); +int REDISMODULE_API_FUNC(RedisModule_RegisterApi)(RedisModuleCtx *ctx, const char *funcname, void *funcptr); +void* REDISMODULE_API_FUNC(RedisModule_GetExportedApi)(RedisModuleCtx *ctx, const char *funcname); #endif /* This is included inline inside each Redis module. */ @@ -492,6 +494,8 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(GetRandomBytes); REDISMODULE_GET_API(GetRandomHexChars); REDISMODULE_GET_API(SetClusterFlags); + REDISMODULE_GET_API(RegisterApi); + REDISMODULE_GET_API(GetExportedApi); #endif if (RedisModule_IsModuleNameBusy && RedisModule_IsModuleNameBusy(name)) return REDISMODULE_ERR; diff --git a/src/server.h b/src/server.h index da4c6d45..379cda05 100644 --- a/src/server.h +++ b/src/server.h @@ -955,6 +955,7 @@ struct redisServer { int always_show_logo; /* Show logo even for non-stdout logging. */ /* Modules */ dict *moduleapi; /* Exported APIs dictionary for modules. */ + dict *exportedapi; /* Api exported by other modules. */ list *loadmodule_queue; /* List of modules to load at startup. */ int module_blocked_pipe[2]; /* Pipe used to awake the event loop if a client blocked on a module command needs From 850b64c1166a1c36e9aa1b12a265b49982d776a0 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 20 Dec 2018 17:56:38 +0100 Subject: [PATCH 07/60] Revert shared APIs to modify the design. --- src/module.c | 115 ---------------------------------------------- src/redismodule.h | 4 -- src/server.h | 1 - 3 files changed, 120 deletions(-) diff --git a/src/module.c b/src/module.c index d177af24..20d159d3 100644 --- a/src/module.c +++ b/src/module.c @@ -47,16 +47,7 @@ struct RedisModule { int ver; /* Module version. We use just progressive integers. */ int apiver; /* Module API version as requested during initialization.*/ list *types; /* Module data types. */ - list *usedBy; /* list of modules names using this module api. */ - list *using; /* list of modules names that this module is using thier api . */ - list *exportedFunctions; /* list of function names exported by this module. */ }; - -struct ModuleExportedApi { - void* funcPointer; - struct RedisModule* module; -}; - typedef struct RedisModule RedisModule; static dict *modules; /* Hash table of modules. SDS -> RedisModule ptr.*/ @@ -709,9 +700,6 @@ void RM_SetModuleAttribs(RedisModuleCtx *ctx, const char *name, int ver, int api module->ver = ver; module->apiver = apiver; module->types = listCreate(); - module->usedBy = listCreate(); - module->using = listCreate(); - module->exportedFunctions = listCreate(); ctx->module = module; } @@ -4627,59 +4615,6 @@ void RM_GetRandomHexChars(char *dst, size_t len) { getRandomHexChars(dst,len); } -/* Used to register an api to be used by other modules. */ -int RM_RegisterApi(RedisModuleCtx *ctx, const char *funcname, void *funcptr) { - struct ModuleExportedApi* eapi = zmalloc(sizeof(*eapi)); - eapi->module = ctx->module; - eapi->funcPointer = funcptr; - if(dictAdd(server.exportedapi, (char*)funcname, eapi) != DICT_OK){ - zfree(eapi); - return REDISMODULE_ERR; - } - listAddNodeHead(ctx->module->exportedFunctions, (char*)funcname); - return REDISMODULE_OK; -} - -static inline int IsModuleInList(list *l, const char* moduleName){ - listIter *iter = listGetIterator(l, AL_START_HEAD); - listNode *node = NULL; - while((node = listNext(iter))){ - char* name = listNodeValue(node); - if(strcmp(name, moduleName) == 0){ - listReleaseIterator(iter); - return 1; - } - } - listReleaseIterator(iter); - return 0; -} - -static inline void RemoveFromList(list *l, const char* moduleName){ - listIter *iter = listGetIterator(l, AL_START_HEAD); - listNode *node = NULL; - while((node = listNext(iter))){ - char* name = listNodeValue(node); - if(strcmp(name, moduleName) == 0){ - listDelNode(l, node); - return; - } - } - listReleaseIterator(iter); -} - -void* RM_GetExportedApi(RedisModuleCtx *ctx, const char *funcname) { - dictEntry* entry = dictFind(server.exportedapi, funcname); - if(!entry){ - return NULL; - } - struct ModuleExportedApi* eapi = dictGetVal(entry); - if(!IsModuleInList(eapi->module->usedBy, ctx->module->name)){ - listAddNodeHead(eapi->module->usedBy, ctx->module->name); - listAddNodeHead(ctx->module->using, eapi->module->name); - } - return eapi->funcPointer; -} - /* -------------------------------------------------------------------------- * Modules API internals * -------------------------------------------------------------------------- */ @@ -4800,28 +4735,6 @@ void moduleUnregisterCommands(struct RedisModule *module) { dictReleaseIterator(di); } -void moduleUnregisterApi(struct RedisModule *module) { - listIter *iter = listGetIterator(module->exportedFunctions, AL_START_HEAD); - listNode* node = NULL; - while((node = listNext(iter))){ - char* functionName = listNodeValue(node); - struct ModuleExportedApi* eapi = dictFetchValue(server.exportedapi, functionName); - serverAssert(eapi); - zfree(eapi); - dictDelete(server.exportedapi, functionName); - } - listReleaseIterator(iter); - iter = listGetIterator(module->using, AL_START_HEAD); - node = NULL; - while((node = listNext(iter))){ - char* moduleName = listNodeValue(node); - struct RedisModule* usingModule = dictFetchValue(modules, moduleName); - serverAssert(usingModule); - RemoveFromList(usingModule->usedBy, module->name); - } - listReleaseIterator(iter); -} - /* Load a module and initialize it. On success C_OK is returned, otherwise * C_ERR is returned. */ int moduleLoad(const char *path, void **module_argv, int module_argc) { @@ -4881,12 +4794,6 @@ int moduleUnload(sds name) { return REDISMODULE_ERR; } - if (listLength(module->usedBy)) { - errno = EPERM; - return REDISMODULE_ERR; - } - - moduleUnregisterApi(module); moduleUnregisterCommands(module); /* Remove any notification subscribers this module might have */ @@ -4919,7 +4826,6 @@ void moduleCommand(client *c) { if (c->argc == 2 && !strcasecmp(subcmd,"help")) { const char *help[] = { "LIST -- Return a list of loaded modules.", -"LISTAPI -- Return a list of exported api.", "LOAD [arg ...] -- Load a module library from .", "UNLOAD -- Unload a module.", NULL @@ -4952,9 +4858,6 @@ NULL case EBUSY: errmsg = "the module exports one or more module-side data types, can't unload"; break; - case EPERM: - errmsg = "the module api is used by other modules, please unload them first and try again."; - break; default: errmsg = "operation not possible."; break; @@ -4976,21 +4879,6 @@ NULL addReplyLongLong(c,module->ver); } dictReleaseIterator(di); - } else if (!strcasecmp(subcmd,"listapi") && c->argc == 3) { - char *moduleName = c->argv[2]->ptr; - struct RedisModule* module = dictFetchValue(modules, moduleName); - if(!module){ - addReplyErrorFormat(c,"Error listing module api: no such module %s", moduleName); - return; - } - addReplyMultiBulkLen(c,listLength(module->exportedFunctions)); - listIter *iter = listGetIterator(module->exportedFunctions, AL_START_HEAD); - listNode* node = NULL; - while((node = listNext(iter))){ - char* functionName = listNodeValue(node); - addReplyBulkCString(c,functionName); - } - listReleaseIterator(iter); } else { addReplySubcommandSyntaxError(c); return; @@ -5006,7 +4894,6 @@ size_t moduleCount(void) { * file so that's easy to seek it to add new entries. */ void moduleRegisterCoreAPI(void) { server.moduleapi = dictCreate(&moduleAPIDictType,NULL); - server.exportedapi = dictCreate(&moduleAPIDictType,NULL); REGISTER_API(Alloc); REGISTER_API(Calloc); REGISTER_API(Realloc); @@ -5157,6 +5044,4 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(DictPrev); REGISTER_API(DictCompareC); REGISTER_API(DictCompare); - REGISTER_API(RegisterApi); - REGISTER_API(GetExportedApi); } diff --git a/src/redismodule.h b/src/redismodule.h index 3c76fa02..d18c3888 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -332,8 +332,6 @@ void REDISMODULE_API_FUNC(RedisModule_GetRandomBytes)(unsigned char *dst, size_t void REDISMODULE_API_FUNC(RedisModule_GetRandomHexChars)(char *dst, size_t len); void REDISMODULE_API_FUNC(RedisModule_SetDisconnectCallback)(RedisModuleBlockedClient *bc, RedisModuleDisconnectFunc callback); void REDISMODULE_API_FUNC(RedisModule_SetClusterFlags)(RedisModuleCtx *ctx, uint64_t flags); -int REDISMODULE_API_FUNC(RedisModule_RegisterApi)(RedisModuleCtx *ctx, const char *funcname, void *funcptr); -void* REDISMODULE_API_FUNC(RedisModule_GetExportedApi)(RedisModuleCtx *ctx, const char *funcname); #endif /* This is included inline inside each Redis module. */ @@ -494,8 +492,6 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(GetRandomBytes); REDISMODULE_GET_API(GetRandomHexChars); REDISMODULE_GET_API(SetClusterFlags); - REDISMODULE_GET_API(RegisterApi); - REDISMODULE_GET_API(GetExportedApi); #endif if (RedisModule_IsModuleNameBusy && RedisModule_IsModuleNameBusy(name)) return REDISMODULE_ERR; diff --git a/src/server.h b/src/server.h index 379cda05..da4c6d45 100644 --- a/src/server.h +++ b/src/server.h @@ -955,7 +955,6 @@ struct redisServer { int always_show_logo; /* Show logo even for non-stdout logging. */ /* Modules */ dict *moduleapi; /* Exported APIs dictionary for modules. */ - dict *exportedapi; /* Api exported by other modules. */ list *loadmodule_queue; /* List of modules to load at startup. */ int module_blocked_pipe[2]; /* Pipe used to awake the event loop if a client blocked on a module command needs From 27f6e9bb9b6614bf4e49d9c53087f21de09cdb1a Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 20 Dec 2018 12:06:24 +0100 Subject: [PATCH 08/60] Modules shared API: initial core functions. Based on ideas and code in PR #5560 by @MeirShpilraien. --- src/module.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/server.h | 4 ++- 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/src/module.c b/src/module.c index 20d159d3..2914b590 100644 --- a/src/module.c +++ b/src/module.c @@ -47,9 +47,21 @@ struct RedisModule { int ver; /* Module version. We use just progressive integers. */ int apiver; /* Module API version as requested during initialization.*/ list *types; /* Module data types. */ + list *usedby; /* List of modules using APIs from this one. */ + list *using; /* List of modules we use some APIs of. */ }; typedef struct RedisModule RedisModule; +/* This represents a shared API. Shared APIs will be used to populate + * the server.sharedapi dictionary, mapping names of APIs exported by + * modules for other modules to use, to their structure specifying the + * function pointer that can be called. */ +struct RedisModuleSharedAPI { + void *func; + RedisModule *module; +}; +typedef struct RedisModuleSharedAPI RedisModuleSharedAPI; + static dict *modules; /* Hash table of modules. SDS -> RedisModule ptr.*/ /* Entries in the context->amqueue array, representing objects to free @@ -700,6 +712,8 @@ void RM_SetModuleAttribs(RedisModuleCtx *ctx, const char *name, int ver, int api module->ver = ver; module->apiver = apiver; module->types = listCreate(); + module->usedby = listCreate(); + module->using = listCreate(); ctx->module = module; } @@ -4615,6 +4629,77 @@ void RM_GetRandomHexChars(char *dst, size_t len) { getRandomHexChars(dst,len); } +/* -------------------------------------------------------------------------- + * Modules API exporting / importing + * -------------------------------------------------------------------------- */ + +/* This function is called by a module in order to export some API with a + * given name. Other modules will be able to use this API by calling the + * symmetrical function RM_GetSharedAPI() and casting the return value to + * the right function pointer. + * + * The function will return REDISMODULE_OK if the name is not already taken, + * otherwise REDISMODULE_ERR will be returned and no operation will be + * performed. + * + * IMPORTANT: the apiname argument should be a string literal with static + * lifetime. The API relies on the fact that it will always be valid in + * the future. */ +int RM_ExportSharedAPI(RedisModuleCtx *ctx, const char *apiname, void *func) { + RedisModuleSharedAPI *sapi = zmalloc(sizeof(*sapi)); + sapi->module = ctx->module; + sapi->func = func; + if (dictAdd(server.sharedapi, (char*)apiname, sapi) != DICT_OK) { + zfree(sapi); + return REDISMODULE_ERR; + } + return REDISMODULE_OK; +} + +/* Request an exported API pointer. The return value is just a void pointer + * that the caller of this function will be required to cast to the right + * function pointer, so this is a private contract between modules. + * + * If the requested API is not available then NULL is returned. Because + * modules can be loaded at different times with different order, this + * function calls should be put inside some module generic API registering + * step, that is called every time a module attempts to execute a + * command that requires external APIs: if some API cannot be resolved, the + * command should return an error. + * + * Here is an exmaple: + * + * int ... myCommandImplementation() { + * if (getExternalAPIs() == 0) { + * reply with an error here if we cannot have the APIs + * } + * // Use the API: + * myFunctionPointer(foo); + * } + * + * And the function registerAPI() is: + * + * int getExternalAPIs(void) { + * static int api_loaded = 0; + * if (api_loaded != 0) return 1; // APIs already resolved. + * + * myFunctionPointer = RedisModule_GetOtherModuleAPI("..."); + * if (myFunctionPointer == NULL) return 0; + * + * return 1; + * } + */ +void *RM_GetSharedAPI(RedisModuleCtx *ctx, const char *apiname) { + dictEntry *de = dictFind(server.sharedapi, apiname); + if (de == NULL) return NULL; + RedisModuleSharedAPI *sapi = dictGetVal(de); + if (listSearchKey(sapi->module->usedby,ctx->module) == NULL) { + listAddNodeTail(sapi->module->usedby,ctx->module); + listAddNodeTail(ctx->module->using,sapi->module); + } + return sapi->func; +} + /* -------------------------------------------------------------------------- * Modules API internals * -------------------------------------------------------------------------- */ @@ -4894,6 +4979,7 @@ size_t moduleCount(void) { * file so that's easy to seek it to add new entries. */ void moduleRegisterCoreAPI(void) { server.moduleapi = dictCreate(&moduleAPIDictType,NULL); + server.sharedapi = dictCreate(&moduleAPIDictType,NULL); REGISTER_API(Alloc); REGISTER_API(Calloc); REGISTER_API(Realloc); diff --git a/src/server.h b/src/server.h index da4c6d45..3c2ecdd2 100644 --- a/src/server.h +++ b/src/server.h @@ -954,7 +954,9 @@ struct redisServer { size_t initial_memory_usage; /* Bytes used after initialization. */ int always_show_logo; /* Show logo even for non-stdout logging. */ /* Modules */ - dict *moduleapi; /* Exported APIs dictionary for modules. */ + dict *moduleapi; /* Exported core APIs dictionary for modules. */ + dict *sharedapi; /* Like moduleapi but containing the APIs that + modules share with each other. */ list *loadmodule_queue; /* List of modules to load at startup. */ int module_blocked_pipe[2]; /* Pipe used to awake the event loop if a client blocked on a module command needs From 6bb8cdaebe74f9c79bb754ccd7a7f05fe8385f81 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 20 Dec 2018 17:16:39 +0100 Subject: [PATCH 09/60] Modules shared API: unregister APIs function. --- src/module.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/module.c b/src/module.c index 2914b590..eafdd81f 100644 --- a/src/module.c +++ b/src/module.c @@ -4700,6 +4700,29 @@ void *RM_GetSharedAPI(RedisModuleCtx *ctx, const char *apiname) { return sapi->func; } +/* Remove all the APIs registered by the specified module. Usually you + * want this when the module is going to be unloaded. This function + * assumes that's caller responsibility to make sure the APIs are not + * used by other modules. + * + * The number of unregistered APIs is returned. */ +int moduleUnregisterSharedAPI(RedisModule *module) { + int count = 0; + dictIterator *di = dictGetSafeIterator(server.sharedapi); + dictEntry *de; + while ((de = dictNext(di)) != NULL) { + const char *apiname = dictGetKey(de); + RedisModuleSharedAPI *sapi = dictGetVal(de); + if (sapi->module == module) { + dictDelete(server.sharedapi,apiname); + zfree(sapi); + count++; + } + } + dictReleaseIterator(di); + return count; +} + /* -------------------------------------------------------------------------- * Modules API internals * -------------------------------------------------------------------------- */ @@ -4843,6 +4866,7 @@ int moduleLoad(const char *path, void **module_argv, int module_argc) { if (onload((void*)&ctx,module_argv,module_argc) == REDISMODULE_ERR) { if (ctx.module) { moduleUnregisterCommands(ctx.module); + moduleUnregisterSharedAPI(ctx.module); moduleFreeModuleStructure(ctx.module); } dlclose(handle); @@ -4880,6 +4904,7 @@ int moduleUnload(sds name) { } moduleUnregisterCommands(module); + moduleUnregisterSharedAPI(module); /* Remove any notification subscribers this module might have */ moduleUnsubscribeNotifications(module); From 9403b3d7a39ea80f95ae71f386d6949f52284426 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 20 Dec 2018 17:31:55 +0100 Subject: [PATCH 10/60] Modules shared API: prevent unloading of used modules. --- src/module.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/module.c b/src/module.c index eafdd81f..4f0e5b12 100644 --- a/src/module.c +++ b/src/module.c @@ -4896,11 +4896,12 @@ int moduleUnload(sds name) { if (module == NULL) { errno = ENOENT; return REDISMODULE_ERR; - } - - if (listLength(module->types)) { + } else if (listLength(module->types)) { errno = EBUSY; return REDISMODULE_ERR; + } else if (listLength(module->usedby)) { + errno = EPERM; + return REDISMODULE_ERR; } moduleUnregisterCommands(module); @@ -4966,7 +4967,12 @@ NULL errmsg = "no such module with that name"; break; case EBUSY: - errmsg = "the module exports one or more module-side data types, can't unload"; + errmsg = "the module exports one or more module-side data " + "types, can't unload"; + break; + case EPERM: + errmsg = "the module exports APIs used by other modules. " + "Please unload them first and try again"; break; default: errmsg = "operation not possible."; From d3eb0028e937fe8c6b435bcb3760f676dcc0920f Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 20 Dec 2018 17:40:55 +0100 Subject: [PATCH 11/60] Modules shared API: also unregister the module as user. --- src/module.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/module.c b/src/module.c index 4f0e5b12..7bb14695 100644 --- a/src/module.c +++ b/src/module.c @@ -4723,6 +4723,27 @@ int moduleUnregisterSharedAPI(RedisModule *module) { return count; } +/* Remove the specified module as an user of APIs of ever other module. + * This is usually called when a module is unloaded. + * + * Returns the number of modules this module was using APIs from. */ +int moduleUnregisterUsedAPI(RedisModule *module) { + listIter li; + listNode *ln; + int count = 0; + + listRewind(module->using,&li); + while((ln = listNext(&li))) { + RedisModule *used = ln->value; + listNode *ln = listSearchKey(used->usedby,module); + if (ln) { + listDelNode(module->using,ln); + count++; + } + } + return count; +} + /* -------------------------------------------------------------------------- * Modules API internals * -------------------------------------------------------------------------- */ @@ -4867,6 +4888,7 @@ int moduleLoad(const char *path, void **module_argv, int module_argc) { if (ctx.module) { moduleUnregisterCommands(ctx.module); moduleUnregisterSharedAPI(ctx.module); + moduleUnregisterUsedAPI(ctx.module); moduleFreeModuleStructure(ctx.module); } dlclose(handle); @@ -4906,6 +4928,7 @@ int moduleUnload(sds name) { moduleUnregisterCommands(module); moduleUnregisterSharedAPI(module); + moduleUnregisterUsedAPI(module); /* Remove any notification subscribers this module might have */ moduleUnsubscribeNotifications(module); From 8a87de130ff9389273516993f9aaec1f75cbb22a Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 20 Dec 2018 17:44:51 +0100 Subject: [PATCH 12/60] Modules shared API: export new core APIs. --- src/module.c | 2 ++ src/redismodule.h | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/src/module.c b/src/module.c index 7bb14695..f2582193 100644 --- a/src/module.c +++ b/src/module.c @@ -5184,4 +5184,6 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(DictPrev); REGISTER_API(DictCompareC); REGISTER_API(DictCompare); + REGISTER_API(ExportSharedAPI); + REGISTER_API(GetSharedAPI); } diff --git a/src/redismodule.h b/src/redismodule.h index d18c3888..3a7da18f 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -332,6 +332,8 @@ void REDISMODULE_API_FUNC(RedisModule_GetRandomBytes)(unsigned char *dst, size_t void REDISMODULE_API_FUNC(RedisModule_GetRandomHexChars)(char *dst, size_t len); void REDISMODULE_API_FUNC(RedisModule_SetDisconnectCallback)(RedisModuleBlockedClient *bc, RedisModuleDisconnectFunc callback); void REDISMODULE_API_FUNC(RedisModule_SetClusterFlags)(RedisModuleCtx *ctx, uint64_t flags); +int REDISMODULE_API_FUNC(RedisModule_ExportSharedAPI)(RedisModuleCtx *ctx, const char *apiname, void *func); +void *REDISMODULE_API_FUNC(RedisModule_GetSharedAPI)(RedisModuleCtx *ctx, const char *apiname); #endif /* This is included inline inside each Redis module. */ @@ -492,6 +494,8 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(GetRandomBytes); REDISMODULE_GET_API(GetRandomHexChars); REDISMODULE_GET_API(SetClusterFlags); + REDISMODULE_GET_API(ExportSharedAPI); + REDISMODULE_GET_API(GetSharedAPI); #endif if (RedisModule_IsModuleNameBusy && RedisModule_IsModuleNameBusy(name)) return REDISMODULE_ERR; From 25029568358e70ac92c6048af4001f4c379ab788 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Wed, 23 Jan 2019 11:11:57 +0200 Subject: [PATCH 13/60] ZPOP should return an empty array if COUNT=0 --- src/t_zset.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/t_zset.c b/src/t_zset.c index 0427ee88..1c3da1a2 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -3140,7 +3140,10 @@ void genericZpopCommand(client *c, robj **keyv, int keyc, int where, int emitkey if (countarg) { if (getLongFromObjectOrReply(c,countarg,&count,NULL) != C_OK) return; - if (count < 0) count = 1; + if (count <= 0) { + addReplyNullArray(c); + return; + } } /* Check type and break on the first error, otherwise identify candidate. */ From b0c8d6c227e172ec93d9b1f1c0f0ac49f8575a8f Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Mon, 28 Jan 2019 17:58:11 +0200 Subject: [PATCH 14/60] Increase string2ld's buffer size (and fix HINCRBYFLOAT) The string representation of `long double` may take up to ~5000 chars (see PR #3745). Before this fix HINCRBYFLOAT would never overflow (since the string could not exceed 256 chars). Now it can. --- src/t_hash.c | 4 ++++ src/util.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/t_hash.c b/src/t_hash.c index d8aee657..bc70e405 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -615,6 +615,10 @@ void hincrbyfloatCommand(client *c) { } value += incr; + if (isnan(value) || isinf(value)) { + addReplyError(c,"increment would produce NaN or Infinity"); + return; + } char buf[MAX_LONG_DOUBLE_CHARS]; int len = ld2string(buf,sizeof(buf),value,1); diff --git a/src/util.c b/src/util.c index 66d59919..783bcf83 100644 --- a/src/util.c +++ b/src/util.c @@ -447,7 +447,7 @@ int string2l(const char *s, size_t slen, long *lval) { * a double: no spaces or other characters before or after the string * representing the number are accepted. */ int string2ld(const char *s, size_t slen, long double *dp) { - char buf[256]; + char buf[MAX_LONG_DOUBLE_CHARS]; long double value; char *eptr; From bdd9a8002a6fcc93135eb4125da703b87a1959fa Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Tue, 12 Feb 2019 14:21:21 +0100 Subject: [PATCH 15/60] Trim SDS free space of retained module strings In some cases processMultibulkBuffer uses sdsMakeRoomFor to expand the querybuf, but later in some cases it uses that query buffer as is for an argv element (see "Optimization"), which means that the sds in argv may have a lot of wasted space, and then in case modules keep that argv RedisString inside their data structure, this space waste will remain for long (until restarted from rdb). --- src/module.c | 11 +++++++++++ src/object.c | 17 ++++++++++++----- src/sds.c | 4 ++++ src/server.h | 1 + 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/module.c b/src/module.c index 81982ba7..5d345191 100644 --- a/src/module.c +++ b/src/module.c @@ -509,6 +509,17 @@ void RedisModuleCommandDispatcher(client *c) { cp->func(&ctx,(void**)c->argv,c->argc); moduleHandlePropagationAfterCommandCallback(&ctx); moduleFreeContext(&ctx); + + /* In some cases processMultibulkBuffer uses sdsMakeRoomFor to + * expand the querybuf, but later in some cases it uses that query + * buffer as is for an argv element (see "Optimization"), which means + * that the sds in argv may have a lot of wasted space, and then in case + * modules keep that argv RedisString inside their data structure, this + * space waste will remain for long (until restarted from rdb). */ + for (int i = 0; i < c->argc; i++) { + if (c->argv[i]->refcount > 1) + trimStringObjectIfNeeded(c->argv[i]); + } } /* This function returns the list of keys, with the same interface as the diff --git a/src/object.c b/src/object.c index ec0bd02e..42c247b8 100644 --- a/src/object.c +++ b/src/object.c @@ -415,6 +415,17 @@ int isObjectRepresentableAsLongLong(robj *o, long long *llval) { } } +void trimStringObjectIfNeeded(robj *o) { + /* Optimize the SDS string inside the string object to require + * little space, in case there is more than 10% of free space + * at the end of the SDS string. */ + if (o->encoding == OBJ_ENCODING_RAW && + sdsavail(o->ptr) > sdslen(o->ptr)/10) + { + o->ptr = sdsRemoveFreeSpace(o->ptr); + } +} + /* Try to encode a string object in order to save space */ robj *tryObjectEncoding(robj *o) { long value; @@ -484,11 +495,7 @@ robj *tryObjectEncoding(robj *o) { * We do that only for relatively large strings as this branch * is only entered if the length of the string is greater than * OBJ_ENCODING_EMBSTR_SIZE_LIMIT. */ - if (o->encoding == OBJ_ENCODING_RAW && - sdsavail(s) > len/10) - { - o->ptr = sdsRemoveFreeSpace(o->ptr); - } + trimStringObjectIfNeeded(o); /* Return the original object. */ return o; diff --git a/src/sds.c b/src/sds.c index 330c955e..cd60946b 100644 --- a/src/sds.c +++ b/src/sds.c @@ -257,8 +257,12 @@ sds sdsRemoveFreeSpace(sds s) { char type, oldtype = s[-1] & SDS_TYPE_MASK; int hdrlen, oldhdrlen = sdsHdrSize(oldtype); size_t len = sdslen(s); + size_t avail = sdsavail(s); sh = (char*)s-oldhdrlen; + /* Return ASAP if there is no space left. */ + if (avail == 0) return s; + /* Check what would be the minimum SDS header that is just good enough to * fit this string. */ type = sdsReqType(len); diff --git a/src/server.h b/src/server.h index a396e1cf..34b4cd8d 100644 --- a/src/server.h +++ b/src/server.h @@ -1656,6 +1656,7 @@ int compareStringObjects(robj *a, robj *b); int collateStringObjects(robj *a, robj *b); int equalStringObjects(robj *a, robj *b); unsigned long long estimateObjectIdleTime(robj *o); +void trimStringObjectIfNeeded(robj *o); #define sdsEncodedObject(objptr) (objptr->encoding == OBJ_ENCODING_RAW || objptr->encoding == OBJ_ENCODING_EMBSTR) /* Synchronous I/O with timeout */ From b2eb48df89d8513a359faa677146d3c36e6266ab Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Thu, 14 Mar 2019 12:11:16 +0100 Subject: [PATCH 16/60] Fix mismatching keyspace notification classes --- src/geo.c | 2 +- src/t_list.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/geo.c b/src/geo.c index 91a0421f..826d11ff 100644 --- a/src/geo.c +++ b/src/geo.c @@ -659,7 +659,7 @@ void georadiusGeneric(client *c, int flags) { zsetConvertToZiplistIfNeeded(zobj,maxelelen); setKey(c->db,storekey,zobj); decrRefCount(zobj); - notifyKeyspaceEvent(NOTIFY_LIST,"georadiusstore",storekey, + notifyKeyspaceEvent(NOTIFY_ZSET,"georadiusstore",storekey, c->db->id); server.dirty += returned_items; } else if (dbDelete(c->db,storekey)) { diff --git a/src/t_list.c b/src/t_list.c index 451ffb4b..45d2e331 100644 --- a/src/t_list.c +++ b/src/t_list.c @@ -520,7 +520,7 @@ void lremCommand(client *c) { if (removed) { signalModifiedKey(c->db,c->argv[1]); - notifyKeyspaceEvent(NOTIFY_GENERIC,"lrem",c->argv[1],c->db->id); + notifyKeyspaceEvent(NOTIFY_LIST,"lrem",c->argv[1],c->db->id); } if (listTypeLength(subject) == 0) { From d292a516181293b54bb7b8d25e0647ae74b5ea62 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 14 Mar 2019 12:47:36 +0100 Subject: [PATCH 17/60] Improve comments after merging #5834. --- src/module.c | 15 ++++++++++----- src/object.c | 7 ++++--- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/module.c b/src/module.c index 28a4d3e6..e69d3dc6 100644 --- a/src/module.c +++ b/src/module.c @@ -523,12 +523,17 @@ void RedisModuleCommandDispatcher(client *c) { moduleFreeContext(&ctx); /* In some cases processMultibulkBuffer uses sdsMakeRoomFor to - * expand the querybuf, but later in some cases it uses that query - * buffer as is for an argv element (see "Optimization"), which means - * that the sds in argv may have a lot of wasted space, and then in case - * modules keep that argv RedisString inside their data structure, this - * space waste will remain for long (until restarted from rdb). */ + * expand the query buffer, and in order to avoid a big object copy + * the query buffer SDS may be used directly as the SDS string backing + * the client argument vectors: sometimes this will result in the SDS + * string having unused space at the end. Later if a module takes ownership + * of the RedisString, such space will be wasted forever. Inside the + * Redis core this is not a problem because tryObjectEncoding() is called + * before storing strings in the key space. Here we need to do it + * for the module. */ for (int i = 0; i < c->argc; i++) { + /* Only do the work if the module took ownership of the object: + * in that case the refcount is no longer 1. */ if (c->argv[i]->refcount > 1) trimStringObjectIfNeeded(c->argv[i]); } diff --git a/src/object.c b/src/object.c index 42c247b8..24e99d19 100644 --- a/src/object.c +++ b/src/object.c @@ -415,10 +415,11 @@ int isObjectRepresentableAsLongLong(robj *o, long long *llval) { } } +/* Optimize the SDS string inside the string object to require little space, + * in case there is more than 10% of free space at the end of the SDS + * string. This happens because SDS strings tend to overallocate to avoid + * wasting too much time in allocations when appending to the string. */ void trimStringObjectIfNeeded(robj *o) { - /* Optimize the SDS string inside the string object to require - * little space, in case there is more than 10% of free space - * at the end of the SDS string. */ if (o->encoding == OBJ_ENCODING_RAW && sdsavail(o->ptr) > sdslen(o->ptr)/10) { From 052e03495f3e6da64d814f80a5dae91721009317 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 14 Mar 2019 17:06:59 +0100 Subject: [PATCH 18/60] Fix objectSetLRUOrLFU() when LFU underflows. --- src/object.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/object.c b/src/object.c index 24e99d19..234e11f8 100644 --- a/src/object.c +++ b/src/object.c @@ -1199,7 +1199,7 @@ sds getMemoryDoctorReport(void) { /* Set the object LRU/LFU depending on server.maxmemory_policy. * The lfu_freq arg is only relevant if policy is MAXMEMORY_FLAG_LFU. - * The lru_idle and lru_clock args are only relevant if policy + * The lru_idle and lru_clock args are only relevant if policy * is MAXMEMORY_FLAG_LRU. * Either or both of them may be <0, in that case, nothing is set. */ void objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, @@ -1210,16 +1210,20 @@ void objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle, val->lru = (LFUGetTimeInMinutes()<<8) | lfu_freq; } } else if (lru_idle >= 0) { - /* Serialized LRU idle time is in seconds. Scale + /* Provided LRU idle time is in seconds. Scale * according to the LRU clock resolution this Redis * instance was compiled with (normally 1000 ms, so the * below statement will expand to lru_idle*1000/1000. */ lru_idle = lru_idle*1000/LRU_CLOCK_RESOLUTION; - val->lru = lru_clock - lru_idle; - /* If the lru field overflows (since LRU it is a wrapping - * clock), the best we can do is to provide the maximum - * representable idle time. */ - if (val->lru < 0) val->lru = lru_clock+1; + long lru_abs = lru_clock - lru_idle; /* Absolute access time. */ + /* If the LRU field underflows (since LRU it is a wrapping + * clock), the best we can do is to provide a large enough LRU + * that is half-way in the circlular LRU clock we use: this way + * the computed idle time for this object will stay high for quite + * some time. */ + if (lru_abs < 0) + lru_abs = (lru_clock+(LRU_CLOCK_MAX/2)) % LRU_CLOCK_MAX; + val->lru = lru_abs; } } From 74d6af8f8094b6d9e2e4bb7ea4eca1941f6412c0 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 14 Mar 2019 17:51:14 +0100 Subject: [PATCH 19/60] Fix ZPOP return type when COUNT=0. Related to #5799. --- src/t_zset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/t_zset.c b/src/t_zset.c index 0daa6d64..fb7078ab 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -3144,7 +3144,7 @@ void genericZpopCommand(client *c, robj **keyv, int keyc, int where, int emitkey if (getLongFromObjectOrReply(c,countarg,&count,NULL) != C_OK) return; if (count <= 0) { - addReplyNullArray(c); + addReply(c,shared.emptyarray); return; } } From a88264d934744b23c02d92a3ba3fccbe070af0b4 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Wed, 30 Nov 2016 21:47:02 +0200 Subject: [PATCH 20/60] Add RedisModule_GetKeyNameFromIO(). --- src/aof.c | 2 +- src/cluster.c | 10 +++++----- src/module.c | 9 +++++++++ src/rdb.c | 14 +++++++------- src/rdb.h | 4 ++-- src/redis-check-rdb.c | 2 +- src/redismodule.h | 2 ++ src/server.h | 4 +++- 8 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/aof.c b/src/aof.c index cafcf961..615eebd0 100644 --- a/src/aof.c +++ b/src/aof.c @@ -1239,7 +1239,7 @@ int rewriteModuleObject(rio *r, robj *key, robj *o) { RedisModuleIO io; moduleValue *mv = o->ptr; moduleType *mt = mv->type; - moduleInitIOContext(io,mt,r); + moduleInitIOContext(io,mt,r,key); mt->aof_rewrite(&io,key,mv->value); if (io.ctx) { moduleFreeContext(io.ctx); diff --git a/src/cluster.c b/src/cluster.c index 50a9ae68..c85e3791 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -4776,7 +4776,7 @@ NULL /* Generates a DUMP-format representation of the object 'o', adding it to the * io stream pointed by 'rio'. This function can't fail. */ -void createDumpPayload(rio *payload, robj *o) { +void createDumpPayload(rio *payload, robj *o, robj *key) { unsigned char buf[2]; uint64_t crc; @@ -4784,7 +4784,7 @@ void createDumpPayload(rio *payload, robj *o) { * byte followed by the serialized object. This is understood by RESTORE. */ rioInitWithBuffer(payload,sdsempty()); serverAssert(rdbSaveObjectType(payload,o)); - serverAssert(rdbSaveObject(payload,o)); + serverAssert(rdbSaveObject(payload,o,key)); /* Write the footer, this is how it looks like: * ----------------+---------------------+---------------+ @@ -4842,7 +4842,7 @@ void dumpCommand(client *c) { } /* Create the DUMP encoded representation. */ - createDumpPayload(&payload,o); + createDumpPayload(&payload,o,c->argv[1]); /* Transfer to the client */ dumpobj = createObject(OBJ_STRING,payload.io.buffer.ptr); @@ -4915,7 +4915,7 @@ void restoreCommand(client *c) { rioInitWithBuffer(&payload,c->argv[3]->ptr); if (((type = rdbLoadObjectType(&payload)) == -1) || - ((obj = rdbLoadObject(type,&payload)) == NULL)) + ((obj = rdbLoadObject(type,&payload,c->argv[1])) == NULL)) { addReplyError(c,"Bad data format"); return; @@ -5203,7 +5203,7 @@ try_again: /* Emit the payload argument, that is the serialized object using * the DUMP format. */ - createDumpPayload(&payload,ov[j]); + createDumpPayload(&payload,ov[j],kv[j]); serverAssertWithInfo(c,NULL, rioWriteBulkString(&cmd,payload.io.buffer.ptr, sdslen(payload.io.buffer.ptr))); diff --git a/src/module.c b/src/module.c index e69d3dc6..e1ffd731 100644 --- a/src/module.c +++ b/src/module.c @@ -3438,6 +3438,14 @@ RedisModuleCtx *RM_GetContextFromIO(RedisModuleIO *io) { return io->ctx; } +/* Returns a RedisModuleString with the name of the key currently saving or + * loading, when an IO data type callback is called. There is no guarantee + * that the key name is always available, so this may return NULL. + */ +const RedisModuleString *RM_GetKeyNameFromIO(RedisModuleIO *io) { + return io->key; +} + /* -------------------------------------------------------------------------- * Logging * -------------------------------------------------------------------------- */ @@ -5164,6 +5172,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(RetainString); REGISTER_API(StringCompare); REGISTER_API(GetContextFromIO); + REGISTER_API(GetKeyNameFromIO); REGISTER_API(BlockClient); REGISTER_API(UnblockClient); REGISTER_API(IsBlockedReplyRequest); diff --git a/src/rdb.c b/src/rdb.c index 52dddf21..95e4766e 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -751,7 +751,7 @@ size_t rdbSaveStreamConsumers(rio *rdb, streamCG *cg) { /* Save a Redis object. * Returns -1 on error, number of bytes written on success. */ -ssize_t rdbSaveObject(rio *rdb, robj *o) { +ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key) { ssize_t n = 0, nwritten = 0; if (o->type == OBJ_STRING) { @@ -966,7 +966,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o) { RedisModuleIO io; moduleValue *mv = o->ptr; moduleType *mt = mv->type; - moduleInitIOContext(io,mt,rdb); + moduleInitIOContext(io,mt,rdb,key); /* Write the "module" identifier as prefix, so that we'll be able * to call the right module during loading. */ @@ -996,7 +996,7 @@ ssize_t rdbSaveObject(rio *rdb, robj *o) { * this length with very little changes to the code. In the future * we could switch to a faster solution. */ size_t rdbSavedObjectLen(robj *o) { - ssize_t len = rdbSaveObject(NULL,o); + ssize_t len = rdbSaveObject(NULL,o,NULL); serverAssertWithInfo(NULL,o,len != -1); return len; } @@ -1038,7 +1038,7 @@ int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime) { /* Save type, key, value */ if (rdbSaveObjectType(rdb,val) == -1) return -1; if (rdbSaveStringObject(rdb,key) == -1) return -1; - if (rdbSaveObject(rdb,val) == -1) return -1; + if (rdbSaveObject(rdb,val,key) == -1) return -1; return 1; } @@ -1380,7 +1380,7 @@ robj *rdbLoadCheckModuleValue(rio *rdb, char *modulename) { /* Load a Redis object of the specified type from the specified file. * On success a newly allocated object is returned, otherwise NULL. */ -robj *rdbLoadObject(int rdbtype, rio *rdb) { +robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { robj *o = NULL, *ele, *dec; uint64_t len; unsigned int i; @@ -1767,7 +1767,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb) { exit(1); } RedisModuleIO io; - moduleInitIOContext(io,mt,rdb); + moduleInitIOContext(io,mt,rdb,key); io.ver = (rdbtype == RDB_TYPE_MODULE) ? 1 : 2; /* Call the rdb_load method of the module providing the 10 bit * encoding version in the lower 10 bits of the module ID. */ @@ -2023,7 +2023,7 @@ int rdbLoadRio(rio *rdb, rdbSaveInfo *rsi, int loading_aof) { /* Read key */ if ((key = rdbLoadStringObject(rdb)) == NULL) goto eoferr; /* Read value */ - if ((val = rdbLoadObject(type,rdb)) == NULL) goto eoferr; + if ((val = rdbLoadObject(type,rdb,key)) == NULL) goto eoferr; /* Check if the key already expired. This function is used when loading * an RDB file from disk, either at startup, or when an RDB was * received from the master. In the latter case, the master is diff --git a/src/rdb.h b/src/rdb.h index 7b948616..0acddf9a 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -140,9 +140,9 @@ int rdbSaveBackground(char *filename, rdbSaveInfo *rsi); int rdbSaveToSlavesSockets(rdbSaveInfo *rsi); void rdbRemoveTempFile(pid_t childpid); int rdbSave(char *filename, rdbSaveInfo *rsi); -ssize_t rdbSaveObject(rio *rdb, robj *o); +ssize_t rdbSaveObject(rio *rdb, robj *o, robj *key); size_t rdbSavedObjectLen(robj *o); -robj *rdbLoadObject(int type, rio *rdb); +robj *rdbLoadObject(int type, rio *rdb, robj *key); void backgroundSaveDoneHandler(int exitcode, int bysignal); int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime); robj *rdbLoadStringObject(rio *rdb); diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index 8de1d8f4..ec00ee71 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -285,7 +285,7 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { rdbstate.keys++; /* Read value */ rdbstate.doing = RDB_CHECK_DOING_READ_OBJECT_VALUE; - if ((val = rdbLoadObject(type,&rdb)) == NULL) goto eoferr; + if ((val = rdbLoadObject(type,&rdb,key)) == NULL) goto eoferr; /* Check if the key already expired. */ if (expiretime != -1 && expiretime < now) rdbstate.already_expired++; diff --git a/src/redismodule.h b/src/redismodule.h index 272da08d..02941aa9 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -278,6 +278,7 @@ int REDISMODULE_API_FUNC(RedisModule_StringAppendBuffer)(RedisModuleCtx *ctx, Re void REDISMODULE_API_FUNC(RedisModule_RetainString)(RedisModuleCtx *ctx, RedisModuleString *str); int REDISMODULE_API_FUNC(RedisModule_StringCompare)(RedisModuleString *a, RedisModuleString *b); RedisModuleCtx *REDISMODULE_API_FUNC(RedisModule_GetContextFromIO)(RedisModuleIO *io); +const RedisModuleString *REDISMODULE_API_FUNC(RedisModule_GetKeyNameFromIO)(RedisModuleIO *io); long long REDISMODULE_API_FUNC(RedisModule_Milliseconds)(void); void REDISMODULE_API_FUNC(RedisModule_DigestAddStringBuffer)(RedisModuleDigest *md, unsigned char *ele, size_t len); void REDISMODULE_API_FUNC(RedisModule_DigestAddLongLong)(RedisModuleDigest *md, long long ele); @@ -442,6 +443,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(RetainString); REDISMODULE_GET_API(StringCompare); REDISMODULE_GET_API(GetContextFromIO); + REDISMODULE_GET_API(GetKeyNameFromIO); REDISMODULE_GET_API(Milliseconds); REDISMODULE_GET_API(DigestAddStringBuffer); REDISMODULE_GET_API(DigestAddLongLong); diff --git a/src/server.h b/src/server.h index 56c3b67d..b888266a 100644 --- a/src/server.h +++ b/src/server.h @@ -578,16 +578,18 @@ typedef struct RedisModuleIO { int ver; /* Module serialization version: 1 (old), * 2 (current version with opcodes annotation). */ struct RedisModuleCtx *ctx; /* Optional context, see RM_GetContextFromIO()*/ + struct redisObject *key; /* Optional name of key processed */ } RedisModuleIO; /* Macro to initialize an IO context. Note that the 'ver' field is populated * inside rdb.c according to the version of the value to load. */ -#define moduleInitIOContext(iovar,mtype,rioptr) do { \ +#define moduleInitIOContext(iovar,mtype,rioptr,keyptr) do { \ iovar.rio = rioptr; \ iovar.type = mtype; \ iovar.bytes = 0; \ iovar.error = 0; \ iovar.ver = 0; \ + iovar.key = keyptr; \ iovar.ctx = NULL; \ } while(0); From 9f13b2bd4967334b1701c6eccdf53760cb13f79e Mon Sep 17 00:00:00 2001 From: John Sully Date: Thu, 14 Mar 2019 14:02:16 -0400 Subject: [PATCH 21/60] Fix hyperloglog corruption --- src/hyperloglog.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/hyperloglog.c b/src/hyperloglog.c index fc21ea00..e993bf26 100644 --- a/src/hyperloglog.c +++ b/src/hyperloglog.c @@ -614,6 +614,10 @@ int hllSparseToDense(robj *o) { } else { runlen = HLL_SPARSE_VAL_LEN(p); regval = HLL_SPARSE_VAL_VALUE(p); + if ((runlen + idx) > HLL_REGISTERS) { + sdsfree(dense); + return C_ERR; + } while(runlen--) { HLL_DENSE_SET_REGISTER(hdr->registers,idx,regval); idx++; @@ -1088,6 +1092,8 @@ int hllMerge(uint8_t *max, robj *hll) { } else { runlen = HLL_SPARSE_VAL_LEN(p); regval = HLL_SPARSE_VAL_VALUE(p); + if ((runlen + i) > HLL_REGISTERS) + return C_ERR; while(runlen--) { if (regval > max[i]) max[i] = regval; i++; From 4208666797b5831eefc022ae46ab5747200cd671 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 15 Mar 2019 13:52:29 +0100 Subject: [PATCH 22/60] HyperLogLog: dense/sparse repr parsing fuzz test. --- tests/unit/hyperloglog.tcl | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/unit/hyperloglog.tcl b/tests/unit/hyperloglog.tcl index 7d36b7a3..6a9c47b1 100644 --- a/tests/unit/hyperloglog.tcl +++ b/tests/unit/hyperloglog.tcl @@ -115,6 +115,35 @@ start_server {tags {"hll"}} { set e } {*WRONGTYPE*} + test {Fuzzing dense/sparse encoding: Redis should always detect errors} { + for {set j 0} {$j < 10000} {incr j} { + r del hll + set items {} + set numitems [randomInt 3000] + for {set i 0} {$i < $numitems} {incr i} { + lappend items [expr {rand()}] + } + r pfadd hll {*}$items + + # Corrupt it in some random way. + for {set i 0} {$i < 5} {incr i} { + set len [r strlen hll] + set pos [randomInt $len] + set byte [randstring 1 1 binary] + r setrange hll $pos $byte + # Don't modify more bytes 50% of times + if {rand() < 0.5} break + } + + # Use the hyperloglog to check if it crashes + # Redis in some way. + catch { + r pfcount hll + r pfdebug getreg hll + } + } + } + test {PFADD, PFCOUNT, PFMERGE type checking works} { r set foo bar catch {r pfadd foo 1} e From a4b90be9fcd5e1668ac941cabce3b1ab38dbe326 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 15 Mar 2019 17:10:16 +0100 Subject: [PATCH 23/60] HyperLogLog: enlarge reghisto variable for safety. --- src/hyperloglog.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/hyperloglog.c b/src/hyperloglog.c index e993bf26..526510b4 100644 --- a/src/hyperloglog.c +++ b/src/hyperloglog.c @@ -1017,7 +1017,12 @@ uint64_t hllCount(struct hllhdr *hdr, int *invalid) { double m = HLL_REGISTERS; double E; int j; - int reghisto[HLL_Q+2] = {0}; + /* Note that reghisto could be just HLL_Q+1, becuase this is the + * maximum frequency of the "000...1" sequence the hash function is + * able to return. However it is slow to check for sanity of the + * input: instead we history array at a safe size: overflows will + * just write data to wrong, but correctly allocated, places. */ + int reghisto[64] = {0}; /* Compute register histogram */ if (hdr->encoding == HLL_DENSE) { From dca7358279bb6449f93e01f7d2806639b8e9ec4b Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 15 Mar 2019 17:13:19 +0100 Subject: [PATCH 24/60] HyperLogLog: speedup fuzz test. --- tests/unit/hyperloglog.tcl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/unit/hyperloglog.tcl b/tests/unit/hyperloglog.tcl index 6a9c47b1..712fcc64 100644 --- a/tests/unit/hyperloglog.tcl +++ b/tests/unit/hyperloglog.tcl @@ -116,7 +116,7 @@ start_server {tags {"hll"}} { } {*WRONGTYPE*} test {Fuzzing dense/sparse encoding: Redis should always detect errors} { - for {set j 0} {$j < 10000} {incr j} { + for {set j 0} {$j < 1000} {incr j} { r del hll set items {} set numitems [randomInt 3000] @@ -139,7 +139,6 @@ start_server {tags {"hll"}} { # Redis in some way. catch { r pfcount hll - r pfdebug getreg hll } } } From e216ceaf0e099536fe3658a29dcb725d812364e0 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 15 Mar 2019 17:16:06 +0100 Subject: [PATCH 25/60] HyperLogLog: handle wrong offset in the base case. --- src/hyperloglog.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/hyperloglog.c b/src/hyperloglog.c index 526510b4..1e7ce3dc 100644 --- a/src/hyperloglog.c +++ b/src/hyperloglog.c @@ -614,10 +614,7 @@ int hllSparseToDense(robj *o) { } else { runlen = HLL_SPARSE_VAL_LEN(p); regval = HLL_SPARSE_VAL_VALUE(p); - if ((runlen + idx) > HLL_REGISTERS) { - sdsfree(dense); - return C_ERR; - } + if ((runlen + idx) > HLL_REGISTERS) break; /* Overflow. */ while(runlen--) { HLL_DENSE_SET_REGISTER(hdr->registers,idx,regval); idx++; @@ -1097,8 +1094,7 @@ int hllMerge(uint8_t *max, robj *hll) { } else { runlen = HLL_SPARSE_VAL_LEN(p); regval = HLL_SPARSE_VAL_VALUE(p); - if ((runlen + i) > HLL_REGISTERS) - return C_ERR; + if ((runlen + i) > HLL_REGISTERS) break; /* Overflow. */ while(runlen--) { if (regval > max[i]) max[i] = regval; i++; From 8ea906a3e8f3e125baa9cf54f6027921d3822b02 Mon Sep 17 00:00:00 2001 From: antirez Date: Sat, 16 Mar 2019 09:15:12 +0100 Subject: [PATCH 26/60] HyperLogLog: fix comment in hllCount(). --- src/hyperloglog.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hyperloglog.c b/src/hyperloglog.c index 1e7ce3dc..e01ea604 100644 --- a/src/hyperloglog.c +++ b/src/hyperloglog.c @@ -1014,8 +1014,8 @@ uint64_t hllCount(struct hllhdr *hdr, int *invalid) { double m = HLL_REGISTERS; double E; int j; - /* Note that reghisto could be just HLL_Q+1, becuase this is the - * maximum frequency of the "000...1" sequence the hash function is + /* Note that reghisto size could be just HLL_Q+2, becuase HLL_Q+1 is + * the maximum frequency of the "000...1" sequence the hash function is * able to return. However it is slow to check for sanity of the * input: instead we history array at a safe size: overflows will * just write data to wrong, but correctly allocated, places. */ From b78ac354f41e370a4dc21ac01981cb0ccd0a1b7d Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 18 Mar 2019 11:15:39 +0100 Subject: [PATCH 27/60] redis-check-aof: fix potential overflow. Bug signaled by @vattezhang in PR #5940 but fixed differently. --- src/redis-check-aof.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/redis-check-aof.c b/src/redis-check-aof.c index c4d5a225..54ed85f0 100644 --- a/src/redis-check-aof.c +++ b/src/redis-check-aof.c @@ -33,8 +33,8 @@ #define ERROR(...) { \ char __buf[1024]; \ - sprintf(__buf, __VA_ARGS__); \ - sprintf(error, "0x%16llx: %s", (long long)epos, __buf); \ + snprintf(__buf, sizeof(__buf), __VA_ARGS__); \ + snprintf(error, sizeof(error), "0x%16llx: %s", (long long)epos, __buf); \ } static char error[1024]; From 14b17c3615108fdbca5e7fe4d2c3f0e8b7454521 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 18 Mar 2019 11:34:40 +0100 Subject: [PATCH 28/60] replicaofCommand() refactoring: stay into 80 cols. --- src/replication.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/replication.c b/src/replication.c index 3c30999a..f2adc799 100644 --- a/src/replication.c +++ b/src/replication.c @@ -2053,8 +2053,11 @@ void replicaofCommand(client *c) { /* Check if we are already attached to the specified slave */ if (server.masterhost && !strcasecmp(server.masterhost,c->argv[1]->ptr) && server.masterport == port) { - serverLog(LL_NOTICE,"REPLICAOF would result into synchronization with the master we are already connected with. No operation performed."); - addReplySds(c,sdsnew("+OK Already connected to specified master\r\n")); + serverLog(LL_NOTICE,"REPLICAOF would result into synchronization " + "with the master we are already connected " + "with. No operation performed."); + addReplySds(c,sdsnew("+OK Already connected to specified " + "master\r\n")); return; } /* There was no previous master or the user specified a different one, From c3e187190b5e48e69f666c8faa2100253a9b536e Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Fri, 23 Feb 2018 16:19:37 +0200 Subject: [PATCH 29/60] Initial command filter experiment. --- src/module.c | 76 +++++++++++++++++++++++++++++++++++++++ src/modules/Makefile | 6 +++- src/modules/hellofilter.c | 69 +++++++++++++++++++++++++++++++++++ src/redismodule.h | 8 +++++ src/server.c | 2 ++ src/server.h | 2 +- 6 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 src/modules/hellofilter.c diff --git a/src/module.c b/src/module.c index e69d3dc6..1780342e 100644 --- a/src/module.c +++ b/src/module.c @@ -270,6 +270,28 @@ typedef struct RedisModuleDictIter { raxIterator ri; } RedisModuleDictIter; +/* Information about the command to be executed, as passed to and from a + * filter. */ +typedef struct RedisModuleFilteredCommand { + RedisModuleString **argv; + int argc; +} RedisModuleFilteredCommand; + +typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCtx *ctx, RedisModuleFilteredCommand *cmd); + +typedef struct RedisModuleCommandFilter { + /* The module that registered the filter */ + RedisModule *module; + /* Filter callback function */ + RedisModuleCommandFilterFunc callback; + /* Indicates a filter is active, avoid reentrancy */ + int active; +} RedisModuleCommandFilter; + +/* Registered filters */ +static list *moduleCommandFilters; + + /* -------------------------------------------------------------------------- * Prototypes * -------------------------------------------------------------------------- */ @@ -4770,6 +4792,56 @@ int moduleUnregisterUsedAPI(RedisModule *module) { return count; } +/* -------------------------------------------------------------------------- + * Module Command Filter API + * -------------------------------------------------------------------------- */ + +/* Register a new command filter function. Filters get executed by Redis + * before processing an inbound command and can be used to manipulate the + * behavior of standard Redis commands. Filters must not attempt to + * perform Redis commands or operate on the dataset, and must restrict + * themselves to manipulation of the arguments. + */ + +int RM_RegisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc callback) { + RedisModuleCommandFilter *filter = zmalloc(sizeof(*filter)); + filter->module = ctx->module; + filter->callback = callback; + filter->active = 0; + + listAddNodeTail(moduleCommandFilters, filter); + return REDISMODULE_OK; +} + +void moduleCallCommandFilters(client *c) { + if (listLength(moduleCommandFilters) == 0) return; + + listIter li; + listNode *ln; + listRewind(moduleCommandFilters,&li); + + RedisModuleFilteredCommand cmd = { + .argv = c->argv, + .argc = c->argc + }; + + while((ln = listNext(&li))) { + RedisModuleCommandFilter *filter = ln->value; + if (filter->active) continue; + + RedisModuleCtx ctx = REDISMODULE_CTX_INIT; + ctx.module = filter->module; + + filter->active = 1; + filter->callback(&ctx, &cmd); + filter->active = 0; + moduleFreeContext(&ctx); + } + + c->argv = cmd.argv; + c->argc = cmd.argc; +} + /* -------------------------------------------------------------------------- * Modules API internals * -------------------------------------------------------------------------- */ @@ -4816,6 +4888,9 @@ void moduleInitModulesSystem(void) { moduleFreeContextReusedClient->flags |= CLIENT_MODULE; moduleFreeContextReusedClient->user = NULL; /* root user. */ + /* Set up filter list */ + moduleCommandFilters = listCreate(); + moduleRegisterCoreAPI(); if (pipe(server.module_blocked_pipe) == -1) { serverLog(LL_WARNING, @@ -5219,4 +5294,5 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(DictCompare); REGISTER_API(ExportSharedAPI); REGISTER_API(GetSharedAPI); + REGISTER_API(RegisterCommandFilter); } diff --git a/src/modules/Makefile b/src/modules/Makefile index 51ffac17..537aa0da 100644 --- a/src/modules/Makefile +++ b/src/modules/Makefile @@ -13,7 +13,7 @@ endif .SUFFIXES: .c .so .xo .o -all: helloworld.so hellotype.so helloblock.so testmodule.so hellocluster.so hellotimer.so hellodict.so +all: helloworld.so hellotype.so helloblock.so testmodule.so hellocluster.so hellotimer.so hellodict.so hellofilter.so .c.xo: $(CC) -I. $(CFLAGS) $(SHOBJ_CFLAGS) -fPIC -c $< -o $@ @@ -46,6 +46,10 @@ hellotimer.so: hellotimer.xo hellodict.xo: ../redismodule.h hellodict.so: hellodict.xo + +hellofilter.xo: ../redismodule.h + +hellofilter.so: hellofilter.xo $(LD) -o $@ $< $(SHOBJ_LDFLAGS) $(LIBS) -lc testmodule.xo: ../redismodule.h diff --git a/src/modules/hellofilter.c b/src/modules/hellofilter.c new file mode 100644 index 00000000..c9e33158 --- /dev/null +++ b/src/modules/hellofilter.c @@ -0,0 +1,69 @@ +#define REDISMODULE_EXPERIMENTAL_API +#include "../redismodule.h" + +static RedisModuleString *log_key_name; + +static const char log_command_name[] = "hellofilter.log"; + +int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +{ + RedisModuleString *s = RedisModule_CreateStringFromString(ctx, argv[0]); + + int i; + for (i = 1; i < argc; i++) { + size_t arglen; + const char *arg = RedisModule_StringPtrLen(argv[i], &arglen); + + RedisModule_StringAppendBuffer(ctx, s, " ", 1); + RedisModule_StringAppendBuffer(ctx, s, arg, arglen); + } + + RedisModuleKey *log = RedisModule_OpenKey(ctx, log_key_name, REDISMODULE_WRITE|REDISMODULE_READ); + RedisModule_ListPush(log, REDISMODULE_LIST_HEAD, s); + RedisModule_CloseKey(log); + RedisModule_FreeString(ctx, s); + + size_t cmdlen; + const char *cmdname = RedisModule_StringPtrLen(argv[1], &cmdlen); + RedisModuleCallReply *reply = RedisModule_Call(ctx, cmdname, "v", &argv[2], argc - 2); + if (reply) { + RedisModule_ReplyWithCallReply(ctx, reply); + RedisModule_FreeCallReply(reply); + } else { + RedisModule_ReplyWithSimpleString(ctx, "Unknown command or invalid arguments"); + } + return REDISMODULE_OK; +} + +void HelloFilter_CommandFilter(RedisModuleCtx *ctx, RedisModuleFilteredCommand *cmd) +{ + cmd->argv = RedisModule_Realloc(cmd->argv, (cmd->argc+1)*sizeof(RedisModuleString *)); + int i; + + for (i = cmd->argc; i > 0; i--) { + cmd->argv[i] = cmd->argv[i-1]; + } + cmd->argv[0] = RedisModule_CreateString(ctx, log_command_name, sizeof(log_command_name)-1); + cmd->argc++; +} + +int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + if (RedisModule_Init(ctx,"hellofilter",1,REDISMODULE_APIVER_1) + == REDISMODULE_ERR) return REDISMODULE_ERR; + + if (argc != 1) { + RedisModule_Log(ctx, "warning", "Log key name not specified"); + return REDISMODULE_ERR; + } + + log_key_name = RedisModule_CreateStringFromString(ctx, argv[0]); + + if (RedisModule_CreateCommand(ctx,log_command_name, + HelloFilter_LogCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) + return REDISMODULE_ERR; + + if (RedisModule_RegisterCommandFilter(ctx, HelloFilter_CommandFilter) + == REDISMODULE_ERR) return REDISMODULE_ERR; + + return REDISMODULE_OK; +} diff --git a/src/redismodule.h b/src/redismodule.h index 272da08d..54ce99d9 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -163,6 +163,12 @@ typedef void (*RedisModuleTypeFreeFunc)(void *value); typedef void (*RedisModuleClusterMessageReceiver)(RedisModuleCtx *ctx, const char *sender_id, uint8_t type, const unsigned char *payload, uint32_t len); typedef void (*RedisModuleTimerProc)(RedisModuleCtx *ctx, void *data); +typedef struct RedisModuleFilteredCommand { + RedisModuleString **argv; + int argc; +} RedisModuleFilteredCommand; +typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCtx *ctx, RedisModuleFilteredCommand *cmd); + #define REDISMODULE_TYPE_METHOD_VERSION 1 typedef struct RedisModuleTypeMethods { uint64_t version; @@ -337,6 +343,7 @@ void REDISMODULE_API_FUNC(RedisModule_SetDisconnectCallback)(RedisModuleBlockedC void REDISMODULE_API_FUNC(RedisModule_SetClusterFlags)(RedisModuleCtx *ctx, uint64_t flags); int REDISMODULE_API_FUNC(RedisModule_ExportSharedAPI)(RedisModuleCtx *ctx, const char *apiname, void *func); void *REDISMODULE_API_FUNC(RedisModule_GetSharedAPI)(RedisModuleCtx *ctx, const char *apiname); +int REDISMODULE_API_FUNC(RedisModule_RegisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc cb); #endif /* This is included inline inside each Redis module. */ @@ -499,6 +506,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(SetClusterFlags); REDISMODULE_GET_API(ExportSharedAPI); REDISMODULE_GET_API(GetSharedAPI); + REDISMODULE_GET_API(RegisterCommandFilter); #endif if (RedisModule_IsModuleNameBusy && RedisModule_IsModuleNameBusy(name)) return REDISMODULE_ERR; diff --git a/src/server.c b/src/server.c index 712cda1b..66e79dea 100644 --- a/src/server.c +++ b/src/server.c @@ -3268,6 +3268,8 @@ void call(client *c, int flags) { * other operations can be performed by the caller. Otherwise * if C_ERR is returned the client was destroyed (i.e. after QUIT). */ int processCommand(client *c) { + moduleCallCommandFilters(c); + /* The QUIT command is handled separately. Normal command procs will * go through checking for replication and QUIT will cause trouble * when FORCE_REPLICATION is enabled and would be implemented in diff --git a/src/server.h b/src/server.h index 56c3b67d..f55213bf 100644 --- a/src/server.h +++ b/src/server.h @@ -1489,7 +1489,7 @@ size_t moduleCount(void); void moduleAcquireGIL(void); void moduleReleaseGIL(void); void moduleNotifyKeyspaceEvent(int type, const char *event, robj *key, int dbid); - +void moduleCallCommandFilters(client *c); /* Utils */ long long ustime(void); From a5af648fdddaf93e89735a8577b56f12379d1dd2 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 18 Mar 2019 15:38:43 +0100 Subject: [PATCH 30/60] MANIFESTO v2. --- MANIFESTO | 47 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/MANIFESTO b/MANIFESTO index 2b719057..d43a5889 100644 --- a/MANIFESTO +++ b/MANIFESTO @@ -34,7 +34,21 @@ Redis Manifesto so that the complexity is obvious and more complex operations can be performed as the sum of the basic operations. -4 - Code is like a poem; it's not just something we write to reach some +4 - We believe in code efficiency. Computers get faster and faster, yet we + believe that abusing computing capabilities is not wise: the amount of + operations you can do for a given amount of energy remains anyway a + significant parameter: it allows to do more with less computers and, at + the same time, having a smaller environmental impact. Similarly Redis is + able to "scale down" to smaller devices. It is perfectly usable in a + Raspberry Pi and other small ARM based computers. Faster code having + just the layers of abstractions that are really needed will also result, + often, in more predictable performances. We think likewise about memory + usage, one of the fundamental goals of the Redis project is to + incrementally build more and more memory efficient data structures, so that + problems that were not approachable in RAM in the past will be perfectly + fine to handle in the future. + +5 - Code is like a poem; it's not just something we write to reach some practical result. Sometimes people that are far from the Redis philosophy suggest using other code written by other authors (frequently in other languages) in order to implement something Redis currently lacks. But to us @@ -45,23 +59,44 @@ Redis Manifesto when needed. At the same time, when writing the Redis story we're trying to write smaller stories that will fit in to other code. -5 - We're against complexity. We believe designing systems is a fight against +6 - We're against complexity. We believe designing systems is a fight against complexity. We'll accept to fight the complexity when it's worthwhile but we'll try hard to recognize when a small feature is not worth 1000s of lines of code. Most of the time the best way to fight complexity is by not creating it at all. -6 - Two levels of API. The Redis API has two levels: 1) a subset of the API fits +7 - Threading is not a silver bullet. Instead of making Redis threaded we + believe on the idea of an efficient (mostly) single threaded Redis core. + Multiple of such cores, that may run in the same computer or may run + in multiple computers, are abstracted away as a single big system by + higher order protocols and features: Redis Cluster and the upcoming + Redis Proxy are our main goals. A shared nothing approach is not just + much simpler (see the previous point in this document), is also optimal + in NUMA systems. In the specific case of Redis it allows for each instance + to have a more limited amount of data, making the Redis persist-by-fork + approach more sounding. In the future we may explore parallelism only for + I/O, which is the low hanging fruit: minimal complexity could provide an + improved single process experience. + +8 - Two levels of API. The Redis API has two levels: 1) a subset of the API fits naturally into a distributed version of Redis and 2) a more complex API that supports multi-key operations. Both are useful if used judiciously but there's no way to make the more complex multi-keys API distributed in an opaque way without violating our other principles. We don't want to provide the illusion of something that will work magically when actually it can't in all cases. Instead we'll provide commands to quickly migrate keys from one - instance to another to perform multi-key operations and expose the tradeoffs - to the user. + instance to another to perform multi-key operations and expose the + trade-offs to the user. -7 - We optimize for joy. We believe writing code is a lot of hard work, and the +9 - We optimize for joy. We believe writing code is a lot of hard work, and the only way it can be worth is by enjoying it. When there is no longer joy in writing code, the best thing to do is stop. To prevent this, we'll avoid taking paths that will make Redis less of a joy to develop. + +10 - All the above points are put together in what we call opportunistic + programming: trying to get the most for the user with minimal increases + in complexity (hanging fruits). Solve 95% of the problem with 5% of the + code when it is acceptable. Avoid a fixed schedule but follow the flow of + user requests, inspiration, Redis internal readiness for certain features + (sometimes many past changes reach a critical point making a previously + complex feature very easy to obtain). From 3eaa2cdc44a9b0742f0695f44911b92547995836 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 18 Mar 2019 15:49:52 +0100 Subject: [PATCH 31/60] MANIFESTO: simplicity and lock-in. --- MANIFESTO | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/MANIFESTO b/MANIFESTO index d43a5889..37278946 100644 --- a/MANIFESTO +++ b/MANIFESTO @@ -63,7 +63,11 @@ Redis Manifesto complexity. We'll accept to fight the complexity when it's worthwhile but we'll try hard to recognize when a small feature is not worth 1000s of lines of code. Most of the time the best way to fight complexity is by not - creating it at all. + creating it at all. Complexity is also a form of lock-in: code that is + very hard to understand cannot be modified by users in an independent way + regardless of the license. One of the main Redis goals is to remain + understandable, enough for a single programmer to have a clear idea of how + it works in detail just reading the source code for a couple of weeks. 7 - Threading is not a silver bullet. Instead of making Redis threaded we believe on the idea of an efficient (mostly) single threaded Redis core. From 67111320835ee46498ea0e4de07dab5cb59584da Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 18 Mar 2019 18:36:46 +0200 Subject: [PATCH 32/60] Add command filtering argument handling API. --- src/module.c | 81 +++++++++++++++++++++++++++++++++++++++ src/modules/hellofilter.c | 46 ++++++++++++++++++---- src/redismodule.h | 18 ++++++--- 3 files changed, 132 insertions(+), 13 deletions(-) diff --git a/src/module.c b/src/module.c index 1780342e..741c546b 100644 --- a/src/module.c +++ b/src/module.c @@ -291,6 +291,10 @@ typedef struct RedisModuleCommandFilter { /* Registered filters */ static list *moduleCommandFilters; +typedef struct RedisModuleCommandFilterCtx { + RedisModuleString **argv; + int argc; +} RedisModuleCommandFilterCtx; /* -------------------------------------------------------------------------- * Prototypes @@ -4842,6 +4846,78 @@ void moduleCallCommandFilters(client *c) { c->argc = cmd.argc; } +/* Return the number of arguments a filtered command has. The number of + * arguments include the command itself. + */ +int RM_CommandFilterArgsCount(RedisModuleCommandFilterCtx *filter) +{ + return filter->argc; +} + +/* Return the specified command argument. The first argument (position 0) is + * the command itself, and the rest are user-provided args. + */ +const RedisModuleString *RM_CommandFilterArgGet(RedisModuleCommandFilterCtx *filter, int pos) +{ + if (pos < 0 || pos >= filter->argc) return NULL; + return filter->argv[pos]; +} + +/* Modify the filtered command by inserting a new argument at the specified + * position. The specified RedisModuleString argument may be used by Redis + * after the filter context is destroyed, so it must not be auto-memory + * allocated, freed or used elsewhere. + */ + +int RM_CommandFilterArgInsert(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg) +{ + int i; + + if (pos < 0 || pos > filter->argc) return REDISMODULE_ERR; + + filter->argv = zrealloc(filter->argv, (filter->argc+1)*sizeof(RedisModuleString *)); + for (i = filter->argc; i > pos; i--) { + filter->argv[i] = filter->argv[i-1]; + } + filter->argv[pos] = arg; + filter->argc++; + + return REDISMODULE_OK; +} + +/* Modify the filtered command by replacing an existing argument with a new one. + * The specified RedisModuleString argument may be used by Redis after the + * filter context is destroyed, so it must not be auto-memory allocated, freed + * or used elsewhere. + */ + +int RM_CommandFilterArgReplace(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg) +{ + if (pos < 0 || pos >= filter->argc) return REDISMODULE_ERR; + + decrRefCount(filter->argv[pos]); + filter->argv[pos] = arg; + + return REDISMODULE_OK; +} + +/* Modify the filtered command by deleting an argument at the specified + * position. + */ +int RM_CommandFilterArgDelete(RedisModuleCommandFilterCtx *filter, int pos) +{ + int i; + if (pos < 0 || pos >= filter->argc) return REDISMODULE_ERR; + + decrRefCount(filter->argv[pos]); + for (i = pos; i < filter->argc-1; i++) { + filter->argv[i] = filter->argv[i+1]; + } + filter->argc--; + + return REDISMODULE_OK; +} + /* -------------------------------------------------------------------------- * Modules API internals * -------------------------------------------------------------------------- */ @@ -5295,4 +5371,9 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(ExportSharedAPI); REGISTER_API(GetSharedAPI); REGISTER_API(RegisterCommandFilter); + REGISTER_API(CommandFilterArgsCount); + REGISTER_API(CommandFilterArgGet); + REGISTER_API(CommandFilterArgInsert); + REGISTER_API(CommandFilterArgReplace); + REGISTER_API(CommandFilterArgDelete); } diff --git a/src/modules/hellofilter.c b/src/modules/hellofilter.c index c9e33158..84eb02c3 100644 --- a/src/modules/hellofilter.c +++ b/src/modules/hellofilter.c @@ -1,6 +1,8 @@ #define REDISMODULE_EXPERIMENTAL_API #include "../redismodule.h" +#include + static RedisModuleString *log_key_name; static const char log_command_name[] = "hellofilter.log"; @@ -35,16 +37,46 @@ int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int ar return REDISMODULE_OK; } -void HelloFilter_CommandFilter(RedisModuleCtx *ctx, RedisModuleFilteredCommand *cmd) +void HelloFilter_CommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterCtx *filter) { - cmd->argv = RedisModule_Realloc(cmd->argv, (cmd->argc+1)*sizeof(RedisModuleString *)); - int i; + (void) ctx; - for (i = cmd->argc; i > 0; i--) { - cmd->argv[i] = cmd->argv[i-1]; + /* Fun manipulations: + * - Remove @delme + * - Replace @replaceme + * - Append @insertbefore or @insertafter + * - Prefix with Log command if @log encounterd + */ + int log = 0; + int pos = 0; + while (pos < RedisModule_CommandFilterArgsCount(filter)) { + const RedisModuleString *arg = RedisModule_CommandFilterArgGet(filter, pos); + size_t arg_len; + const char *arg_str = RedisModule_StringPtrLen(arg, &arg_len); + + if (arg_len == 6 && !memcmp(arg_str, "@delme", 6)) { + RedisModule_CommandFilterArgDelete(filter, pos); + continue; + } + if (arg_len == 10 && !memcmp(arg_str, "@replaceme", 10)) { + RedisModule_CommandFilterArgReplace(filter, pos, + RedisModule_CreateString(NULL, "--replaced--", 12)); + } else if (arg_len == 13 && !memcmp(arg_str, "@insertbefore", 13)) { + RedisModule_CommandFilterArgInsert(filter, pos, + RedisModule_CreateString(NULL, "--inserted-before--", 19)); + pos++; + } else if (arg_len == 12 && !memcmp(arg_str, "@insertafter", 12)) { + RedisModule_CommandFilterArgInsert(filter, pos + 1, + RedisModule_CreateString(NULL, "--inserted-after--", 18)); + pos++; + } else if (arg_len == 4 && !memcmp(arg_str, "@log", 4)) { + log = 1; + } + pos++; } - cmd->argv[0] = RedisModule_CreateString(ctx, log_command_name, sizeof(log_command_name)-1); - cmd->argc++; + + if (log) RedisModule_CommandFilterArgInsert(filter, 0, + RedisModule_CreateString(NULL, log_command_name, sizeof(log_command_name)-1)); } int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { diff --git a/src/redismodule.h b/src/redismodule.h index 54ce99d9..426a6df6 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -150,6 +150,7 @@ typedef struct RedisModuleBlockedClient RedisModuleBlockedClient; typedef struct RedisModuleClusterInfo RedisModuleClusterInfo; typedef struct RedisModuleDict RedisModuleDict; typedef struct RedisModuleDictIter RedisModuleDictIter; +typedef struct RedisModuleCommandFilterCtx RedisModuleCommandFilterCtx; typedef int (*RedisModuleCmdFunc)(RedisModuleCtx *ctx, RedisModuleString **argv, int argc); typedef void (*RedisModuleDisconnectFunc)(RedisModuleCtx *ctx, RedisModuleBlockedClient *bc); @@ -162,12 +163,7 @@ typedef void (*RedisModuleTypeDigestFunc)(RedisModuleDigest *digest, void *value typedef void (*RedisModuleTypeFreeFunc)(void *value); typedef void (*RedisModuleClusterMessageReceiver)(RedisModuleCtx *ctx, const char *sender_id, uint8_t type, const unsigned char *payload, uint32_t len); typedef void (*RedisModuleTimerProc)(RedisModuleCtx *ctx, void *data); - -typedef struct RedisModuleFilteredCommand { - RedisModuleString **argv; - int argc; -} RedisModuleFilteredCommand; -typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCtx *ctx, RedisModuleFilteredCommand *cmd); +typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCtx *ctx, RedisModuleCommandFilterCtx *filter); #define REDISMODULE_TYPE_METHOD_VERSION 1 typedef struct RedisModuleTypeMethods { @@ -344,6 +340,11 @@ void REDISMODULE_API_FUNC(RedisModule_SetClusterFlags)(RedisModuleCtx *ctx, uint int REDISMODULE_API_FUNC(RedisModule_ExportSharedAPI)(RedisModuleCtx *ctx, const char *apiname, void *func); void *REDISMODULE_API_FUNC(RedisModule_GetSharedAPI)(RedisModuleCtx *ctx, const char *apiname); int REDISMODULE_API_FUNC(RedisModule_RegisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc cb); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgsCount)(RedisModuleCommandFilterCtx *filter); +const RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CommandFilterArgGet)(RedisModuleCommandFilterCtx *filter, int pos); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgInsert)(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgReplace)(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgDelete)(RedisModuleCommandFilterCtx *filter, int pos); #endif /* This is included inline inside each Redis module. */ @@ -507,6 +508,11 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(ExportSharedAPI); REDISMODULE_GET_API(GetSharedAPI); REDISMODULE_GET_API(RegisterCommandFilter); + REDISMODULE_GET_API(CommandFilterArgsCount); + REDISMODULE_GET_API(CommandFilterArgGet); + REDISMODULE_GET_API(CommandFilterArgInsert); + REDISMODULE_GET_API(CommandFilterArgReplace); + REDISMODULE_GET_API(CommandFilterArgDelete); #endif if (RedisModule_IsModuleNameBusy && RedisModule_IsModuleNameBusy(name)) return REDISMODULE_ERR; From 9095e4dc9bbb8c0311e0df2af556295ca6ce92ca Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 18 Mar 2019 19:34:52 +0200 Subject: [PATCH 33/60] Add command filter Module API tests. --- tests/modules/commandfilter.tcl | 27 +++++++++++++++++++++++++++ tests/test_helper.tcl | 1 + 2 files changed, 28 insertions(+) create mode 100644 tests/modules/commandfilter.tcl diff --git a/tests/modules/commandfilter.tcl b/tests/modules/commandfilter.tcl new file mode 100644 index 00000000..f0d96b25 --- /dev/null +++ b/tests/modules/commandfilter.tcl @@ -0,0 +1,27 @@ +set testmodule [file normalize src/modules/hellofilter.so] + +start_server {tags {"modules"}} { + r module load $testmodule log-key + + test {Command Filter handles redirected commands} { + r set mykey @log + r lrange log-key 0 -1 + } "{hellofilter.log set mykey @log}" + + test {Command Filter can call RedisModule_CommandFilterArgDelete} { + r rpush mylist elem1 @delme elem2 + r lrange mylist 0 -1 + } {elem1 elem2} + + test {Command Filter can call RedisModule_CommandFilterArgInsert} { + r del mylist + r rpush mylist elem1 @insertbefore elem2 @insertafter elem3 + r lrange mylist 0 -1 + } {elem1 --inserted-before-- @insertbefore elem2 @insertafter --inserted-after-- elem3} + + test {Command Filter can call RedisModule_CommandFilterArgReplace} { + r del mylist + r rpush mylist elem1 @replaceme elem2 + r lrange mylist 0 -1 + } {elem1 --replaced-- elem2} +} diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 568eacde..d2f28152 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -63,6 +63,7 @@ set ::all_tests { unit/lazyfree unit/wait unit/pendingquerybuf + modules/commandfilter } # Index to the next test to run in the ::all_tests list. set ::next_test 0 From 2a5aeef79f894b80024d49ec1036ac03ae7ac5c5 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 18 Mar 2019 23:05:52 +0200 Subject: [PATCH 34/60] CommandFilter API: More cleanup. --- src/module.c | 37 +++++++++---------------------------- src/redismodule.h | 2 +- 2 files changed, 10 insertions(+), 29 deletions(-) diff --git a/src/module.c b/src/module.c index 741c546b..c6cb8a0c 100644 --- a/src/module.c +++ b/src/module.c @@ -270,32 +270,23 @@ typedef struct RedisModuleDictIter { raxIterator ri; } RedisModuleDictIter; -/* Information about the command to be executed, as passed to and from a - * filter. */ -typedef struct RedisModuleFilteredCommand { +typedef struct RedisModuleCommandFilterCtx { RedisModuleString **argv; int argc; -} RedisModuleFilteredCommand; +} RedisModuleCommandFilterCtx; -typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCtx *ctx, RedisModuleFilteredCommand *cmd); +typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCommandFilterCtx *filter); typedef struct RedisModuleCommandFilter { /* The module that registered the filter */ RedisModule *module; /* Filter callback function */ RedisModuleCommandFilterFunc callback; - /* Indicates a filter is active, avoid reentrancy */ - int active; } RedisModuleCommandFilter; /* Registered filters */ static list *moduleCommandFilters; -typedef struct RedisModuleCommandFilterCtx { - RedisModuleString **argv; - int argc; -} RedisModuleCommandFilterCtx; - /* -------------------------------------------------------------------------- * Prototypes * -------------------------------------------------------------------------- */ @@ -4802,16 +4793,13 @@ int moduleUnregisterUsedAPI(RedisModule *module) { /* Register a new command filter function. Filters get executed by Redis * before processing an inbound command and can be used to manipulate the - * behavior of standard Redis commands. Filters must not attempt to - * perform Redis commands or operate on the dataset, and must restrict - * themselves to manipulation of the arguments. + * behavior of standard Redis commands. */ int RM_RegisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc callback) { RedisModuleCommandFilter *filter = zmalloc(sizeof(*filter)); filter->module = ctx->module; filter->callback = callback; - filter->active = 0; listAddNodeTail(moduleCommandFilters, filter); return REDISMODULE_OK; @@ -4824,26 +4812,19 @@ void moduleCallCommandFilters(client *c) { listNode *ln; listRewind(moduleCommandFilters,&li); - RedisModuleFilteredCommand cmd = { + RedisModuleCommandFilterCtx filter = { .argv = c->argv, .argc = c->argc }; while((ln = listNext(&li))) { - RedisModuleCommandFilter *filter = ln->value; - if (filter->active) continue; + RedisModuleCommandFilter *f = ln->value; - RedisModuleCtx ctx = REDISMODULE_CTX_INIT; - ctx.module = filter->module; - - filter->active = 1; - filter->callback(&ctx, &cmd); - filter->active = 0; - moduleFreeContext(&ctx); + f->callback(&filter); } - c->argv = cmd.argv; - c->argc = cmd.argc; + c->argv = filter.argv; + c->argc = filter.argc; } /* Return the number of arguments a filtered command has. The number of diff --git a/src/redismodule.h b/src/redismodule.h index 426a6df6..5df83ae6 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -163,7 +163,7 @@ typedef void (*RedisModuleTypeDigestFunc)(RedisModuleDigest *digest, void *value typedef void (*RedisModuleTypeFreeFunc)(void *value); typedef void (*RedisModuleClusterMessageReceiver)(RedisModuleCtx *ctx, const char *sender_id, uint8_t type, const unsigned char *payload, uint32_t len); typedef void (*RedisModuleTimerProc)(RedisModuleCtx *ctx, void *data); -typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCtx *ctx, RedisModuleCommandFilterCtx *filter); +typedef void (*RedisModuleCommandFilterFunc) (RedisModuleCommandFilterCtx *filter); #define REDISMODULE_TYPE_METHOD_VERSION 1 typedef struct RedisModuleTypeMethods { From 325fc1cb2e2e15a99e5d012184d177dc19257036 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 18 Mar 2019 23:06:38 +0200 Subject: [PATCH 35/60] CommandFilter API: Support Lua and RM_call() flows. --- src/module.c | 20 +++++++++++++------- src/scripting.c | 5 +++++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/module.c b/src/module.c index c6cb8a0c..17accfb7 100644 --- a/src/module.c +++ b/src/module.c @@ -2741,12 +2741,6 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch RedisModuleCallReply *reply = NULL; int replicate = 0; /* Replicate this command? */ - cmd = lookupCommandByCString((char*)cmdname); - if (!cmd) { - errno = EINVAL; - return NULL; - } - /* Create the client and dispatch the command. */ va_start(ap, fmt); c = createClient(-1); @@ -2760,11 +2754,23 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch c->db = ctx->client->db; c->argv = argv; c->argc = argc; - c->cmd = c->lastcmd = cmd; /* We handle the above format error only when the client is setup so that * we can free it normally. */ if (argv == NULL) goto cleanup; + /* Call command filters */ + moduleCallCommandFilters(c); + + /* Lookup command now, after filters had a chance to make modifications + * if necessary. + */ + cmd = lookupCommand(c->argv[0]->ptr); + if (!cmd) { + errno = EINVAL; + goto cleanup; + } + c->cmd = c->lastcmd = cmd; + /* Basic arity checks. */ if ((cmd->arity > 0 && cmd->arity != argc) || (argc < -cmd->arity)) { errno = EINVAL; diff --git a/src/scripting.c b/src/scripting.c index cbbf43fb..032bfdf1 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -462,6 +462,11 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { c->argc = argc; c->user = server.lua_caller->user; + /* Process module hooks */ + moduleCallCommandFilters(c); + argv = c->argv; + argc = c->argc; + /* Log the command if debugging is active. */ if (ldb.active && ldb.step) { sds cmdlog = sdsnew(""); From a9a6a894e82442600f11d97d23f70b90316ca0a4 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 18 Mar 2019 23:07:28 +0200 Subject: [PATCH 36/60] CommandFilter API: hellofilter and tests. --- src/modules/hellofilter.c | 32 ++++++++++++++++++++++++++++---- tests/modules/commandfilter.tcl | 20 +++++++++++++++++++- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/modules/hellofilter.c b/src/modules/hellofilter.c index 84eb02c3..d5dd405a 100644 --- a/src/modules/hellofilter.c +++ b/src/modules/hellofilter.c @@ -6,17 +6,32 @@ static RedisModuleString *log_key_name; static const char log_command_name[] = "hellofilter.log"; +static const char ping_command_name[] = "hellofilter.ping"; +static int in_module = 0; + +int HelloFilter_PingCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +{ + RedisModuleCallReply *reply = RedisModule_Call(ctx, "ping", "c", "@log"); + if (reply) { + RedisModule_ReplyWithCallReply(ctx, reply); + RedisModule_FreeCallReply(reply); + } else { + RedisModule_ReplyWithSimpleString(ctx, "Unknown command or invalid arguments"); + } + + return REDISMODULE_OK; +} int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { - RedisModuleString *s = RedisModule_CreateStringFromString(ctx, argv[0]); + RedisModuleString *s = RedisModule_CreateString(ctx, "", 0); int i; for (i = 1; i < argc; i++) { size_t arglen; const char *arg = RedisModule_StringPtrLen(argv[i], &arglen); - RedisModule_StringAppendBuffer(ctx, s, " ", 1); + if (i > 1) RedisModule_StringAppendBuffer(ctx, s, " ", 1); RedisModule_StringAppendBuffer(ctx, s, arg, arglen); } @@ -25,6 +40,8 @@ int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int ar RedisModule_CloseKey(log); RedisModule_FreeString(ctx, s); + in_module = 1; + size_t cmdlen; const char *cmdname = RedisModule_StringPtrLen(argv[1], &cmdlen); RedisModuleCallReply *reply = RedisModule_Call(ctx, cmdname, "v", &argv[2], argc - 2); @@ -34,12 +51,15 @@ int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int ar } else { RedisModule_ReplyWithSimpleString(ctx, "Unknown command or invalid arguments"); } + + in_module = 0; + return REDISMODULE_OK; } -void HelloFilter_CommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterCtx *filter) +void HelloFilter_CommandFilter(RedisModuleCommandFilterCtx *filter) { - (void) ctx; + if (in_module) return; /* don't process our own RM_Call() */ /* Fun manipulations: * - Remove @delme @@ -94,6 +114,10 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) HelloFilter_LogCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,ping_command_name, + HelloFilter_PingCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) + return REDISMODULE_ERR; + if (RedisModule_RegisterCommandFilter(ctx, HelloFilter_CommandFilter) == REDISMODULE_ERR) return REDISMODULE_ERR; diff --git a/tests/modules/commandfilter.tcl b/tests/modules/commandfilter.tcl index f0d96b25..47d9c302 100644 --- a/tests/modules/commandfilter.tcl +++ b/tests/modules/commandfilter.tcl @@ -6,7 +6,7 @@ start_server {tags {"modules"}} { test {Command Filter handles redirected commands} { r set mykey @log r lrange log-key 0 -1 - } "{hellofilter.log set mykey @log}" + } "{set mykey @log}" test {Command Filter can call RedisModule_CommandFilterArgDelete} { r rpush mylist elem1 @delme elem2 @@ -24,4 +24,22 @@ start_server {tags {"modules"}} { r rpush mylist elem1 @replaceme elem2 r lrange mylist 0 -1 } {elem1 --replaced-- elem2} + + test {Command Filter applies on RM_Call() commands} { + r del log-key + r hellofilter.ping + r lrange log-key 0 -1 + } "{ping @log}" + + test {Command Filter applies on Lua redis.call()} { + r del log-key + r eval "redis.call('ping', '@log')" 0 + r lrange log-key 0 -1 + } "{ping @log}" + + test {Command Filter applies on Lua redis.call() that calls a module} { + r del log-key + r eval "redis.call('hellofilter.ping')" 0 + r lrange log-key 0 -1 + } "{ping @log}" } From 8620a434a058aa5c66cccf2cc571e4337c73d12b Mon Sep 17 00:00:00 2001 From: Dvir Volk Date: Tue, 19 Mar 2019 13:11:37 +0200 Subject: [PATCH 37/60] Added keyspace miss notifications support --- src/db.c | 7 ++++++- src/modules/testmodule.c | 29 ++++++++++++++++++++++------- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/db.c b/src/db.c index 7950d507..8c904708 100644 --- a/src/db.c +++ b/src/db.c @@ -83,6 +83,7 @@ robj *lookupKey(redisDb *db, robj *key, int flags) { * 1. A key gets expired if it reached it's TTL. * 2. The key last access time is updated. * 3. The global keys hits/misses stats are updated (reported in INFO). + * 4. If keyspace notifications are enabled, a "miss" notification is fired. * * This API should not be used when we write to the key after obtaining * the object linked to the key, but only for read only operations. @@ -106,6 +107,7 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) { * to return NULL ASAP. */ if (server.masterhost == NULL) { server.stat_keyspace_misses++; + notifyKeyspaceEvent(NOTIFY_GENERIC, "miss", key, db->id); return NULL; } @@ -127,12 +129,15 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) { server.current_client->cmd->flags & CMD_READONLY) { server.stat_keyspace_misses++; + notifyKeyspaceEvent(NOTIFY_GENERIC, "miss", key, db->id); return NULL; } } val = lookupKey(db,key,flags); - if (val == NULL) + if (val == NULL) { server.stat_keyspace_misses++; + notifyKeyspaceEvent(NOTIFY_GENERIC, "miss", key, db->id); + } else server.stat_keyspace_hits++; return val; diff --git a/src/modules/testmodule.c b/src/modules/testmodule.c index 67a86170..826dd9a7 100644 --- a/src/modules/testmodule.c +++ b/src/modules/testmodule.c @@ -109,9 +109,9 @@ int TestStringPrintf(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { if (argc < 3) { return RedisModule_WrongArity(ctx); } - RedisModuleString *s = RedisModule_CreateStringPrintf(ctx, - "Got %d args. argv[1]: %s, argv[2]: %s", - argc, + RedisModuleString *s = RedisModule_CreateStringPrintf(ctx, + "Got %d args. argv[1]: %s, argv[2]: %s", + argc, RedisModule_StringPtrLen(argv[1], NULL), RedisModule_StringPtrLen(argv[2], NULL) ); @@ -133,7 +133,7 @@ int TestUnlink(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { RedisModuleKey *k = RedisModule_OpenKey(ctx, RedisModule_CreateStringPrintf(ctx, "unlinked"), REDISMODULE_WRITE | REDISMODULE_READ); if (!k) return failTest(ctx, "Could not create key"); - + if (REDISMODULE_ERR == RedisModule_StringSet(k, RedisModule_CreateStringPrintf(ctx, "Foobar"))) { return failTest(ctx, "Could not set string value"); } @@ -152,7 +152,7 @@ int TestUnlink(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { return failTest(ctx, "Could not verify key to be unlinked"); } return RedisModule_ReplyWithSimpleString(ctx, "OK"); - + } int NotifyCallback(RedisModuleCtx *ctx, int type, const char *event, @@ -188,6 +188,10 @@ int TestNotifications(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { RedisModule_Call(ctx, "LPUSH", "cc", "l", "y"); RedisModule_Call(ctx, "LPUSH", "cc", "l", "y"); + /* Miss some keys intentionally so we will get a "miss" notification. */ + RedisModule_Call(ctx, "GET", "c", "nosuchkey"); + RedisModule_Call(ctx, "SMEMBERS", "c", "nosuchkey"); + size_t sz; const char *rep; RedisModuleCallReply *r = RedisModule_Call(ctx, "HGET", "cc", "notifications", "foo"); @@ -225,6 +229,16 @@ int TestNotifications(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { FAIL("Wrong reply for l"); } + r = RedisModule_Call(ctx, "HGET", "cc", "notifications", "nosuchkey"); + if (r == NULL || RedisModule_CallReplyType(r) != REDISMODULE_REPLY_STRING) { + FAIL("Wrong or no reply for nosuchkey"); + } else { + rep = RedisModule_CallReplyStringPtr(r, &sz); + if (sz != 1 || *rep != '2') { + FAIL("Got reply '%.*s'. expected '2'", sz, rep); + } + } + RedisModule_Call(ctx, "FLUSHDB", ""); return RedisModule_ReplyWithSimpleString(ctx, "OK"); @@ -423,7 +437,7 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if (RedisModule_CreateCommand(ctx,"test.ctxflags", TestCtxFlags,"readonly",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; - + if (RedisModule_CreateCommand(ctx,"test.unlink", TestUnlink,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; @@ -435,7 +449,8 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) RedisModule_SubscribeToKeyspaceEvents(ctx, REDISMODULE_NOTIFY_HASH | REDISMODULE_NOTIFY_SET | - REDISMODULE_NOTIFY_STRING, + REDISMODULE_NOTIFY_STRING | + REDISMODULE_NOTIFY_GENERIC, NotifyCallback); if (RedisModule_CreateCommand(ctx,"test.notify", TestNotifications,"write deny-oom",1,1,1) == REDISMODULE_ERR) From dd8b4be46baf86dc4f5e2c787a72b9d31faecdc0 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Tue, 19 Mar 2019 19:48:47 +0200 Subject: [PATCH 38/60] CommandFilter API: Extend documentation. --- src/module.c | 48 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/src/module.c b/src/module.c index 17accfb7..ee284022 100644 --- a/src/module.c +++ b/src/module.c @@ -4797,9 +4797,47 @@ int moduleUnregisterUsedAPI(RedisModule *module) { * Module Command Filter API * -------------------------------------------------------------------------- */ -/* Register a new command filter function. Filters get executed by Redis - * before processing an inbound command and can be used to manipulate the - * behavior of standard Redis commands. +/* Register a new command filter function. + * + * Command filtering makes it possible for modules to extend Redis by plugging + * into the execution flow of all commands. + * + * A registered filter gets called before Redis executes *any* command. This + * includes both core Redis commands and commands registered by any module. The + * filter applies in all execution paths including: + * + * 1. Invocation by a client. + * 2. Invocation through `RedisModule_Call()` by any module. + * 3. Invocation through Lua 'redis.call()`. + * 4. Replication of a command from a master. + * + * The filter executes in a special filter context, which is different and more + * limited than a RedisModuleCtx. Because the filter affects any command, it + * must be implemented in a very efficient way to reduce the performance impact + * on Redis. All Redis Module API calls that require a valid context (such as + * `RedisModule_Call()`, `RedisModule_OpenKey()`, etc.) are not supported in a + * filter context. + * + * The `RedisModuleCommandFilterCtx` can be used to inspect or modify the + * executed command and its arguments. As the filter executes before Redis + * begins processing the command, any change will affect the way the command is + * processed. For example, a module can override Redis commands this way: + * + * 1. Register a `MODULE.SET` command which implements an extended version of + * the Redis `SET` command. + * 2. Register a command filter which detects invocation of `SET` on a specific + * pattern of keys. Once detected, the filter will replace the first + * argument from `SET` to `MODULE.SET`. + * 3. When filter execution is complete, Redis considers the new command name + * and therefore executes the module's own command. + * + * Note that in the above use case, if `MODULE.SET` itself uses + * `RedisModule_Call()` the filter will be applied on that call as well. If + * that is not desired, the module itself is responsible for maintaining a flag + * to identify and avoid this form of re-entrancy. + * + * If multiple filters are registered (by the same or different modules), they + * are executed in the order of registration. */ int RM_RegisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc callback) { @@ -4881,7 +4919,7 @@ int RM_CommandFilterArgInsert(RedisModuleCommandFilterCtx *filter, int pos, Redi int RM_CommandFilterArgReplace(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg) { if (pos < 0 || pos >= filter->argc) return REDISMODULE_ERR; - + decrRefCount(filter->argv[pos]); filter->argv[pos] = arg; @@ -4901,7 +4939,7 @@ int RM_CommandFilterArgDelete(RedisModuleCommandFilterCtx *filter, int pos) filter->argv[i] = filter->argv[i+1]; } filter->argc--; - + return REDISMODULE_OK; } From 385f6190a3a9f8d2d5775bd058aaa2173dc05c8c Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 27 Sep 2018 18:12:31 +0300 Subject: [PATCH 39/60] getKeysFromCommand for TOUCH only extracted the first key. also, airty for COMMAND command was wrong. --- src/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index 712cda1b..1341ab40 100644 --- a/src/server.c +++ b/src/server.c @@ -715,7 +715,7 @@ struct redisCommand redisCommandTable[] = { {"touch",touchCommand,-2, "read-only fast @keyspace", - 0,NULL,1,1,1,0,0,0}, + 0,NULL,1,-1,1,0,0,0}, {"pttl",pttlCommand,2, "read-only fast random @keyspace", @@ -863,7 +863,7 @@ struct redisCommand redisCommandTable[] = { "no-script @keyspace", 0,NULL,0,0,0,0,0,0}, - {"command",commandCommand,0, + {"command",commandCommand,-1, "ok-loading ok-stale random @connection", 0,NULL,0,0,0,0,0,0}, From 747174388f305148b0832dd97b9754e2a64bdfef Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 27 Sep 2018 18:03:47 +0300 Subject: [PATCH 40/60] change SORT and SPOP to use lookupKeyWrite rather than lookupKeyRead like in SUNIONSTORE etc, commands that perform writes are expected to open all keys, even input keys, with lookupKeyWrite --- src/sort.c | 55 ++++++++++++++++++++++++++++++----------------------- src/t_set.c | 2 +- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/sort.c b/src/sort.c index 8608cd8b..db26da15 100644 --- a/src/sort.c +++ b/src/sort.c @@ -58,7 +58,7 @@ redisSortOperation *createSortOperation(int type, robj *pattern) { * * The returned object will always have its refcount increased by 1 * when it is non-NULL. */ -robj *lookupKeyByPattern(redisDb *db, robj *pattern, robj *subst) { +robj *lookupKeyByPattern(redisDb *db, robj *pattern, robj *subst, int writeflag) { char *p, *f, *k; sds spat, ssub; robj *keyobj, *fieldobj = NULL, *o; @@ -106,7 +106,10 @@ robj *lookupKeyByPattern(redisDb *db, robj *pattern, robj *subst) { decrRefCount(subst); /* Incremented by decodeObject() */ /* Lookup substituted key */ - o = lookupKeyRead(db,keyobj); + if (!writeflag) + o = lookupKeyRead(db,keyobj); + else + o = lookupKeyWrite(db,keyobj); if (o == NULL) goto noobj; if (fieldobj) { @@ -198,30 +201,12 @@ void sortCommand(client *c) { robj *sortval, *sortby = NULL, *storekey = NULL; redisSortObject *vector; /* Resulting vector to sort */ - /* Lookup the key to sort. It must be of the right types */ - sortval = lookupKeyRead(c->db,c->argv[1]); - if (sortval && sortval->type != OBJ_SET && - sortval->type != OBJ_LIST && - sortval->type != OBJ_ZSET) - { - addReply(c,shared.wrongtypeerr); - return; - } - /* Create a list of operations to perform for every sorted element. * Operations can be GET */ operations = listCreate(); listSetFreeMethod(operations,zfree); j = 2; /* options start at argv[2] */ - /* Now we need to protect sortval incrementing its count, in the future - * SORT may have options able to overwrite/delete keys during the sorting - * and the sorted key itself may get destroyed */ - if (sortval) - incrRefCount(sortval); - else - sortval = createQuicklistObject(); - /* The SORT command has an SQL-alike syntax, parse it */ while(j < c->argc) { int leftargs = c->argc-j-1; @@ -280,11 +265,33 @@ void sortCommand(client *c) { /* Handle syntax errors set during options parsing. */ if (syntax_error) { - decrRefCount(sortval); listRelease(operations); return; } + /* Lookup the key to sort. It must be of the right types */ + if (storekey) + sortval = lookupKeyRead(c->db,c->argv[1]); + else + sortval = lookupKeyWrite(c->db,c->argv[1]); + if (sortval && sortval->type != OBJ_SET && + sortval->type != OBJ_LIST && + sortval->type != OBJ_ZSET) + { + listRelease(operations); + addReply(c,shared.wrongtypeerr); + return; + } + + /* Now we need to protect sortval incrementing its count, in the future + * SORT may have options able to overwrite/delete keys during the sorting + * and the sorted key itself may get destroyed */ + if (sortval) + incrRefCount(sortval); + else + sortval = createQuicklistObject(); + + /* When sorting a set with no sort specified, we must sort the output * so the result is consistent across scripting and replication. * @@ -452,7 +459,7 @@ void sortCommand(client *c) { robj *byval; if (sortby) { /* lookup value to sort by */ - byval = lookupKeyByPattern(c->db,sortby,vector[j].obj); + byval = lookupKeyByPattern(c->db,sortby,vector[j].obj,storekey!=NULL); if (!byval) continue; } else { /* use object itself to sort by */ @@ -515,7 +522,7 @@ void sortCommand(client *c) { while((ln = listNext(&li))) { redisSortOperation *sop = ln->value; robj *val = lookupKeyByPattern(c->db,sop->pattern, - vector[j].obj); + vector[j].obj,storekey!=NULL); if (sop->type == SORT_OP_GET) { if (!val) { @@ -545,7 +552,7 @@ void sortCommand(client *c) { while((ln = listNext(&li))) { redisSortOperation *sop = ln->value; robj *val = lookupKeyByPattern(c->db,sop->pattern, - vector[j].obj); + vector[j].obj,storekey!=NULL); if (sop->type == SORT_OP_GET) { if (!val) val = createStringObject("",0); diff --git a/src/t_set.c b/src/t_set.c index cbe55aaa..05d9ee24 100644 --- a/src/t_set.c +++ b/src/t_set.c @@ -415,7 +415,7 @@ void spopWithCountCommand(client *c) { /* Make sure a key with the name inputted exists, and that it's type is * indeed a set. Otherwise, return nil */ - if ((set = lookupKeyReadOrReply(c,c->argv[1],shared.null[c->resp])) + if ((set = lookupKeyWriteOrReply(c,c->argv[1],shared.null[c->resp])) == NULL || checkType(c,set,OBJ_SET)) return; /* If count is zero, serve an empty multibulk ASAP to avoid special From c9e2900efc1ed33727356df114fb716442ae2ce6 Mon Sep 17 00:00:00 2001 From: oranagra Date: Thu, 23 Feb 2017 03:13:44 -0800 Subject: [PATCH 41/60] bugfix to restartAOF, exit will never happen since retry will get negative. also reduce an excess sleep --- src/replication.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/replication.c b/src/replication.c index f2adc799..59e42e56 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1091,12 +1091,13 @@ void replicationCreateMasterClient(int fd, int dbid) { } void restartAOF() { - int retry = 10; - while (retry-- && startAppendOnly() == C_ERR) { + unsigned int tries, max_tries = 10; + for (tries = 0; tries < max_tries; ++tries) { + if (tries) sleep(1); + if (startAppendOnly() == C_OK) break; serverLog(LL_WARNING,"Failed enabling the AOF after successful master synchronization! Trying it again in one second."); - sleep(1); } - if (!retry) { + if (tries == max_tries) { serverLog(LL_WARNING,"FATAL: this replica instance finished the synchronization with its master, but the AOF can't be turned on. Exiting now."); exit(1); } From b2e03f83292e65602a6c7dcaad1f6977f39f0b30 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 20 Mar 2019 17:46:19 +0200 Subject: [PATCH 42/60] diskless replication - notify slave when rdb transfer failed in diskless replication - master was not notifing the slave that rdb transfer terminated on error, and lets slave wait for replication timeout --- src/replication.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/replication.c b/src/replication.c index f2adc799..8f0d6791 100644 --- a/src/replication.c +++ b/src/replication.c @@ -593,6 +593,7 @@ int startBgsaveForReplication(int mincapa) { client *slave = ln->value; if (slave->replstate == SLAVE_STATE_WAIT_BGSAVE_START) { + slave->replstate = REPL_STATE_NONE; slave->flags &= ~CLIENT_SLAVE; listDelNode(server.slaves,ln); addReplyError(slave, From 99c2fe0bcf9876daf774fa7df4939cadc7972129 Mon Sep 17 00:00:00 2001 From: Dvir Volk Date: Thu, 21 Mar 2019 11:47:14 +0200 Subject: [PATCH 43/60] added special flag for keyspace miss notifications --- src/db.c | 6 +++--- src/modules/testmodule.c | 2 +- src/notify.c | 6 ++++-- src/redismodule.h | 1 + src/server.h | 4 +++- 5 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/db.c b/src/db.c index 8c904708..afe18128 100644 --- a/src/db.c +++ b/src/db.c @@ -107,7 +107,7 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) { * to return NULL ASAP. */ if (server.masterhost == NULL) { server.stat_keyspace_misses++; - notifyKeyspaceEvent(NOTIFY_GENERIC, "miss", key, db->id); + notifyKeyspaceEvent(NOTIFY_KEY_MISS, "miss", key, db->id); return NULL; } @@ -129,14 +129,14 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) { server.current_client->cmd->flags & CMD_READONLY) { server.stat_keyspace_misses++; - notifyKeyspaceEvent(NOTIFY_GENERIC, "miss", key, db->id); + notifyKeyspaceEvent(NOTIFY_KEY_MISS, "miss", key, db->id); return NULL; } } val = lookupKey(db,key,flags); if (val == NULL) { server.stat_keyspace_misses++; - notifyKeyspaceEvent(NOTIFY_GENERIC, "miss", key, db->id); + notifyKeyspaceEvent(NOTIFY_KEY_MISS, "miss", key, db->id); } else server.stat_keyspace_hits++; diff --git a/src/modules/testmodule.c b/src/modules/testmodule.c index 826dd9a7..af78d21d 100644 --- a/src/modules/testmodule.c +++ b/src/modules/testmodule.c @@ -450,7 +450,7 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) REDISMODULE_NOTIFY_HASH | REDISMODULE_NOTIFY_SET | REDISMODULE_NOTIFY_STRING | - REDISMODULE_NOTIFY_GENERIC, + REDISMODULE_NOTIFY_KEY_MISS, NotifyCallback); if (RedisModule_CreateCommand(ctx,"test.notify", TestNotifications,"write deny-oom",1,1,1) == REDISMODULE_ERR) diff --git a/src/notify.c b/src/notify.c index 1afb36fc..d6c3ad40 100644 --- a/src/notify.c +++ b/src/notify.c @@ -55,6 +55,7 @@ int keyspaceEventsStringToFlags(char *classes) { case 'K': flags |= NOTIFY_KEYSPACE; break; case 'E': flags |= NOTIFY_KEYEVENT; break; case 't': flags |= NOTIFY_STREAM; break; + case 'm': flags |= NOTIFY_KEY_MISS; break; default: return -1; } } @@ -81,6 +82,7 @@ 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); @@ -100,12 +102,12 @@ void notifyKeyspaceEvent(int type, char *event, robj *key, int dbid) { int len = -1; char buf[24]; - /* If any modules are interested in events, notify the module system now. + /* If any modules are interested in events, notify the module system now. * This bypasses the notifications configuration, but the module engine * will only call event subscribers if the event type matches the types * they are interested in. */ moduleNotifyKeyspaceEvent(type, event, key, dbid); - + /* If notifications for this class of events are off, return ASAP. */ if (!(server.notify_keyspace_events & type)) return; diff --git a/src/redismodule.h b/src/redismodule.h index 272da08d..681bd600 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -98,6 +98,7 @@ #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) /* A */ diff --git a/src/server.h b/src/server.h index 56c3b67d..0b433039 100644 --- a/src/server.h +++ b/src/server.h @@ -468,7 +468,9 @@ typedef long long mstime_t; /* millisecond time type. */ #define NOTIFY_EXPIRED (1<<8) /* x */ #define NOTIFY_EVICTED (1<<9) /* e */ #define NOTIFY_STREAM (1<<10) /* t */ -#define NOTIFY_ALL (NOTIFY_GENERIC | NOTIFY_STRING | NOTIFY_LIST | NOTIFY_SET | NOTIFY_HASH | NOTIFY_ZSET | NOTIFY_EXPIRED | NOTIFY_EVICTED | NOTIFY_STREAM) /* A flag */ +#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 */ /* Get the first bind addr or NULL */ #define NET_FIRST_BIND_ADDR (server.bindaddr_count ? server.bindaddr[0] : NULL) From 4a0ee5c6ac7908ee41e69c2d7ace55f698d94418 Mon Sep 17 00:00:00 2001 From: Dvir Volk Date: Thu, 21 Mar 2019 12:47:51 +0200 Subject: [PATCH 44/60] Added missing REDISMODULE_NOTIFY_KEY_MISS flag to REDISMODULE_NOTIFY_ALL --- src/redismodule.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/redismodule.h b/src/redismodule.h index 681bd600..70011f93 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -99,7 +99,7 @@ #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) /* A */ +#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 */ /* A special pointer that we can use between the core and the module to signal From bc269c85e1d7ff15a377aff5197a1a670c65aab9 Mon Sep 17 00:00:00 2001 From: Dvir Volk Date: Thu, 21 Mar 2019 12:48:37 +0200 Subject: [PATCH 45/60] remove extra linebreak --- src/server.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/server.h b/src/server.h index 0b433039..b090a637 100644 --- a/src/server.h +++ b/src/server.h @@ -469,7 +469,6 @@ typedef long long mstime_t; /* millisecond time type. */ #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 */ /* Get the first bind addr or NULL */ From 9dabbd1ab072f3abced48b4995d9ef3e745f0608 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 21 Mar 2019 12:18:55 +0100 Subject: [PATCH 46/60] Alter coding style in #4696 to conform to Redis code base. --- src/zmalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zmalloc.c b/src/zmalloc.c index 4c40a778..5e601027 100644 --- a/src/zmalloc.c +++ b/src/zmalloc.c @@ -148,7 +148,7 @@ void *zrealloc(void *ptr, size_t size) { size_t oldsize; void *newptr; - if (size == 0 && ptr!=NULL) { + if (size == 0 && ptr != NULL) { zfree(ptr); return NULL; } From e2626f69eccc7addf9283285a6849f798e882af8 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Thu, 21 Mar 2019 14:44:49 +0200 Subject: [PATCH 47/60] CommandFilter API: Add unregister option. A filter handle is returned and can be used to unregister a filter. In the future it can also be used to further configure or manipulate the filter. Filters are now automatically unregistered when a module unloads. --- src/module.c | 94 +++++++++++++++++++++++++-------- src/modules/hellofilter.c | 27 ++++++++-- src/redismodule.h | 15 +++--- tests/modules/commandfilter.tcl | 22 ++++++++ 4 files changed, 126 insertions(+), 32 deletions(-) diff --git a/src/module.c b/src/module.c index ee284022..ad7bba2e 100644 --- a/src/module.c +++ b/src/module.c @@ -49,6 +49,7 @@ struct RedisModule { list *types; /* Module data types. */ list *usedby; /* List of modules using APIs from this one. */ list *using; /* List of modules we use some APIs of. */ + list *filters; /* List of filters the module has registered. */ }; typedef struct RedisModule RedisModule; @@ -748,6 +749,7 @@ void RM_SetModuleAttribs(RedisModuleCtx *ctx, const char *name, int ver, int api module->types = listCreate(); module->usedby = listCreate(); module->using = listCreate(); + module->filters = listCreate(); ctx->module = module; } @@ -4793,6 +4795,28 @@ int moduleUnregisterUsedAPI(RedisModule *module) { return count; } +/* Unregister all filters registered by a module. + * This is called when a module is being unloaded. + * + * Returns the number of filters unregistered. */ +int moduleUnregisterFilters(RedisModule *module) { + listIter li; + listNode *ln; + int count = 0; + + listRewind(module->filters,&li); + while((ln = listNext(&li))) { + RedisModuleCommandFilter *filter = ln->value; + listNode *ln = listSearchKey(moduleCommandFilters,filter); + if (ln) { + listDelNode(moduleCommandFilters,ln); + count++; + } + zfree(filter); + } + return count; +} + /* -------------------------------------------------------------------------- * Module Command Filter API * -------------------------------------------------------------------------- */ @@ -4840,12 +4864,33 @@ int moduleUnregisterUsedAPI(RedisModule *module) { * are executed in the order of registration. */ -int RM_RegisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc callback) { +RedisModuleCommandFilter *RM_RegisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc callback) { RedisModuleCommandFilter *filter = zmalloc(sizeof(*filter)); filter->module = ctx->module; filter->callback = callback; listAddNodeTail(moduleCommandFilters, filter); + listAddNodeTail(ctx->module->filters, filter); + return filter; +} + +/* Unregister a command filter. + */ +int RM_UnregisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilter *filter) { + listNode *ln; + + /* A module can only remove its own filters */ + if (filter->module != ctx->module) return REDISMODULE_ERR; + + ln = listSearchKey(moduleCommandFilters,filter); + if (!ln) return REDISMODULE_ERR; + listDelNode(moduleCommandFilters,ln); + + ln = listSearchKey(ctx->module->filters,filter); + if (ln) { + listDelNode(moduleCommandFilters,ln); + } + return REDISMODULE_OK; } @@ -4874,18 +4919,18 @@ void moduleCallCommandFilters(client *c) { /* Return the number of arguments a filtered command has. The number of * arguments include the command itself. */ -int RM_CommandFilterArgsCount(RedisModuleCommandFilterCtx *filter) +int RM_CommandFilterArgsCount(RedisModuleCommandFilterCtx *fctx) { - return filter->argc; + return fctx->argc; } /* Return the specified command argument. The first argument (position 0) is * the command itself, and the rest are user-provided args. */ -const RedisModuleString *RM_CommandFilterArgGet(RedisModuleCommandFilterCtx *filter, int pos) +const RedisModuleString *RM_CommandFilterArgGet(RedisModuleCommandFilterCtx *fctx, int pos) { - if (pos < 0 || pos >= filter->argc) return NULL; - return filter->argv[pos]; + if (pos < 0 || pos >= fctx->argc) return NULL; + return fctx->argv[pos]; } /* Modify the filtered command by inserting a new argument at the specified @@ -4894,18 +4939,18 @@ const RedisModuleString *RM_CommandFilterArgGet(RedisModuleCommandFilterCtx *fil * allocated, freed or used elsewhere. */ -int RM_CommandFilterArgInsert(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg) +int RM_CommandFilterArgInsert(RedisModuleCommandFilterCtx *fctx, int pos, RedisModuleString *arg) { int i; - if (pos < 0 || pos > filter->argc) return REDISMODULE_ERR; + if (pos < 0 || pos > fctx->argc) return REDISMODULE_ERR; - filter->argv = zrealloc(filter->argv, (filter->argc+1)*sizeof(RedisModuleString *)); - for (i = filter->argc; i > pos; i--) { - filter->argv[i] = filter->argv[i-1]; + fctx->argv = zrealloc(fctx->argv, (fctx->argc+1)*sizeof(RedisModuleString *)); + for (i = fctx->argc; i > pos; i--) { + fctx->argv[i] = fctx->argv[i-1]; } - filter->argv[pos] = arg; - filter->argc++; + fctx->argv[pos] = arg; + fctx->argc++; return REDISMODULE_OK; } @@ -4916,12 +4961,12 @@ int RM_CommandFilterArgInsert(RedisModuleCommandFilterCtx *filter, int pos, Redi * or used elsewhere. */ -int RM_CommandFilterArgReplace(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg) +int RM_CommandFilterArgReplace(RedisModuleCommandFilterCtx *fctx, int pos, RedisModuleString *arg) { - if (pos < 0 || pos >= filter->argc) return REDISMODULE_ERR; + if (pos < 0 || pos >= fctx->argc) return REDISMODULE_ERR; - decrRefCount(filter->argv[pos]); - filter->argv[pos] = arg; + decrRefCount(fctx->argv[pos]); + fctx->argv[pos] = arg; return REDISMODULE_OK; } @@ -4929,16 +4974,16 @@ int RM_CommandFilterArgReplace(RedisModuleCommandFilterCtx *filter, int pos, Red /* Modify the filtered command by deleting an argument at the specified * position. */ -int RM_CommandFilterArgDelete(RedisModuleCommandFilterCtx *filter, int pos) +int RM_CommandFilterArgDelete(RedisModuleCommandFilterCtx *fctx, int pos) { int i; - if (pos < 0 || pos >= filter->argc) return REDISMODULE_ERR; + if (pos < 0 || pos >= fctx->argc) return REDISMODULE_ERR; - decrRefCount(filter->argv[pos]); - for (i = pos; i < filter->argc-1; i++) { - filter->argv[i] = filter->argv[i+1]; + decrRefCount(fctx->argv[pos]); + for (i = pos; i < fctx->argc-1; i++) { + fctx->argv[i] = fctx->argv[i+1]; } - filter->argc--; + fctx->argc--; return REDISMODULE_OK; } @@ -5041,6 +5086,7 @@ void moduleLoadFromQueue(void) { void moduleFreeModuleStructure(struct RedisModule *module) { listRelease(module->types); + listRelease(module->filters); sdsfree(module->name); zfree(module); } @@ -5132,6 +5178,7 @@ int moduleUnload(sds name) { moduleUnregisterCommands(module); moduleUnregisterSharedAPI(module); moduleUnregisterUsedAPI(module); + moduleUnregisterFilters(module); /* Remove any notification subscribers this module might have */ moduleUnsubscribeNotifications(module); @@ -5396,6 +5443,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(ExportSharedAPI); REGISTER_API(GetSharedAPI); REGISTER_API(RegisterCommandFilter); + REGISTER_API(UnregisterCommandFilter); REGISTER_API(CommandFilterArgsCount); REGISTER_API(CommandFilterArgGet); REGISTER_API(CommandFilterArgInsert); diff --git a/src/modules/hellofilter.c b/src/modules/hellofilter.c index d5dd405a..9cd440df 100644 --- a/src/modules/hellofilter.c +++ b/src/modules/hellofilter.c @@ -7,10 +7,27 @@ static RedisModuleString *log_key_name; static const char log_command_name[] = "hellofilter.log"; static const char ping_command_name[] = "hellofilter.ping"; +static const char unregister_command_name[] = "hellofilter.unregister"; static int in_module = 0; +static RedisModuleCommandFilter *filter = NULL; + +int HelloFilter_UnregisterCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +{ + (void) argc; + (void) argv; + + RedisModule_ReplyWithLongLong(ctx, + RedisModule_UnregisterCommandFilter(ctx, filter)); + + return REDISMODULE_OK; +} + int HelloFilter_PingCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + (void) argc; + (void) argv; + RedisModuleCallReply *reply = RedisModule_Call(ctx, "ping", "c", "@log"); if (reply) { RedisModule_ReplyWithCallReply(ctx, reply); @@ -115,11 +132,15 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) return REDISMODULE_ERR; if (RedisModule_CreateCommand(ctx,ping_command_name, - HelloFilter_PingCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) + HelloFilter_PingCommand,"deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if (RedisModule_RegisterCommandFilter(ctx, HelloFilter_CommandFilter) - == REDISMODULE_ERR) return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,unregister_command_name, + HelloFilter_UnregisterCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) + return REDISMODULE_ERR; + + if ((filter = RedisModule_RegisterCommandFilter(ctx, HelloFilter_CommandFilter)) + == NULL) return REDISMODULE_ERR; return REDISMODULE_OK; } diff --git a/src/redismodule.h b/src/redismodule.h index 5df83ae6..37b7d0d5 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -151,6 +151,7 @@ typedef struct RedisModuleClusterInfo RedisModuleClusterInfo; typedef struct RedisModuleDict RedisModuleDict; typedef struct RedisModuleDictIter RedisModuleDictIter; typedef struct RedisModuleCommandFilterCtx RedisModuleCommandFilterCtx; +typedef struct RedisModuleCommandFilter RedisModuleCommandFilter; typedef int (*RedisModuleCmdFunc)(RedisModuleCtx *ctx, RedisModuleString **argv, int argc); typedef void (*RedisModuleDisconnectFunc)(RedisModuleCtx *ctx, RedisModuleBlockedClient *bc); @@ -339,12 +340,13 @@ void REDISMODULE_API_FUNC(RedisModule_SetDisconnectCallback)(RedisModuleBlockedC void REDISMODULE_API_FUNC(RedisModule_SetClusterFlags)(RedisModuleCtx *ctx, uint64_t flags); int REDISMODULE_API_FUNC(RedisModule_ExportSharedAPI)(RedisModuleCtx *ctx, const char *apiname, void *func); void *REDISMODULE_API_FUNC(RedisModule_GetSharedAPI)(RedisModuleCtx *ctx, const char *apiname); -int REDISMODULE_API_FUNC(RedisModule_RegisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc cb); -int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgsCount)(RedisModuleCommandFilterCtx *filter); -const RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CommandFilterArgGet)(RedisModuleCommandFilterCtx *filter, int pos); -int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgInsert)(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg); -int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgReplace)(RedisModuleCommandFilterCtx *filter, int pos, RedisModuleString *arg); -int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgDelete)(RedisModuleCommandFilterCtx *filter, int pos); +RedisModuleCommandFilter *REDISMODULE_API_FUNC(RedisModule_RegisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc cb); +int REDISMODULE_API_FUNC(RedisModule_UnregisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilter *filter); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgsCount)(RedisModuleCommandFilterCtx *fctx); +const RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CommandFilterArgGet)(RedisModuleCommandFilterCtx *fctx, int pos); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgInsert)(RedisModuleCommandFilterCtx *fctx, int pos, RedisModuleString *arg); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgReplace)(RedisModuleCommandFilterCtx *fctx, int pos, RedisModuleString *arg); +int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgDelete)(RedisModuleCommandFilterCtx *fctx, int pos); #endif /* This is included inline inside each Redis module. */ @@ -508,6 +510,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(ExportSharedAPI); REDISMODULE_GET_API(GetSharedAPI); REDISMODULE_GET_API(RegisterCommandFilter); + REDISMODULE_GET_API(UnregisterCommandFilter); REDISMODULE_GET_API(CommandFilterArgsCount); REDISMODULE_GET_API(CommandFilterArgGet); REDISMODULE_GET_API(CommandFilterArgInsert); diff --git a/tests/modules/commandfilter.tcl b/tests/modules/commandfilter.tcl index 47d9c302..8645d827 100644 --- a/tests/modules/commandfilter.tcl +++ b/tests/modules/commandfilter.tcl @@ -42,4 +42,26 @@ start_server {tags {"modules"}} { r eval "redis.call('hellofilter.ping')" 0 r lrange log-key 0 -1 } "{ping @log}" + + test {Command Filter is unregistered implicitly on module unload} { + r del log-key + r module unload hellofilter + r set mykey @log + r lrange log-key 0 -1 + } {} + + r module load $testmodule log-key-2 + + test {Command Filter unregister works as expected} { + # Validate reloading succeeded + r set mykey @log + assert_equal "{set mykey @log}" [r lrange log-key-2 0 -1] + + # Unregister + r hellofilter.unregister + r del log-key-2 + + r set mykey @log + r lrange log-key-2 0 -1 + } {} } From 9588fd52ac3333d0bf3243523ec9a165fa18f87e Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 21 Mar 2019 17:18:24 +0100 Subject: [PATCH 48/60] Mostly aesthetic changes to restartAOF(). See #3829. --- src/replication.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/replication.c b/src/replication.c index 59e42e56..c25e7fa6 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1093,12 +1093,16 @@ void replicationCreateMasterClient(int fd, int dbid) { void restartAOF() { unsigned int tries, max_tries = 10; for (tries = 0; tries < max_tries; ++tries) { - if (tries) sleep(1); if (startAppendOnly() == C_OK) break; - serverLog(LL_WARNING,"Failed enabling the AOF after successful master synchronization! Trying it again in one second."); + serverLog(LL_WARNING, + "Failed enabling the AOF after successful master synchronization! " + "Trying it again in one second."); + sleep(1); } if (tries == max_tries) { - serverLog(LL_WARNING,"FATAL: this replica instance finished the synchronization with its master, but the AOF can't be turned on. Exiting now."); + serverLog(LL_WARNING, + "FATAL: this replica instance finished the synchronization with " + "its master, but the AOF can't be turned on. Exiting now."); exit(1); } } From b3408e9a9b1bdf8ea59bf80d715c695a113820f3 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 21 Mar 2019 17:21:25 +0100 Subject: [PATCH 49/60] More sensible name for function: restartAOFAfterSYNC(). Related to #3829. --- src/replication.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/replication.c b/src/replication.c index c25e7fa6..a27c29a3 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1090,7 +1090,11 @@ void replicationCreateMasterClient(int fd, int dbid) { if (dbid != -1) selectDb(server.master,dbid); } -void restartAOF() { +/* This function will try to re-enable the AOF file after the + * master-replica synchronization: if it fails after multiple attempts + * the replica cannot be considered reliable and exists with an + * error. */ +void restartAOFAfterSYNC() { unsigned int tries, max_tries = 10; for (tries = 0; tries < max_tries; ++tries) { if (startAppendOnly() == C_OK) break; @@ -1289,7 +1293,7 @@ void readSyncBulkPayload(aeEventLoop *el, int fd, void *privdata, int mask) { cancelReplicationHandshake(); /* Re-enable the AOF if we disabled it earlier, in order to restore * the original configuration. */ - if (aof_is_enabled) restartAOF(); + if (aof_is_enabled) restartAOFAfterSYNC(); return; } /* Final setup of the connected slave <- master link */ @@ -1314,7 +1318,7 @@ void readSyncBulkPayload(aeEventLoop *el, int fd, void *privdata, int mask) { /* Restart the AOF subsystem now that we finished the sync. This * will trigger an AOF rewrite, and when done will start appending * to the new file. */ - if (aof_is_enabled) restartAOF(); + if (aof_is_enabled) restartAOFAfterSYNC(); } return; From 4ea3ed896b286c8f2bf192e07e1c36802a3a1c38 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Thu, 21 Mar 2019 19:45:41 +0200 Subject: [PATCH 50/60] CommandFilter API: fix UnregisterCommandFilter. --- src/module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/module.c b/src/module.c index ad7bba2e..a54bd1ad 100644 --- a/src/module.c +++ b/src/module.c @@ -4887,9 +4887,8 @@ int RM_UnregisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilter *fi listDelNode(moduleCommandFilters,ln); ln = listSearchKey(ctx->module->filters,filter); - if (ln) { - listDelNode(moduleCommandFilters,ln); - } + if (!ln) return REDISMODULE_ERR; /* Shouldn't happen */ + listDelNode(ctx->module->filters,ln); return REDISMODULE_OK; } From 6c0a5fde3d0d90c85f086ca955f9473fe41797b3 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Thu, 21 Mar 2019 19:47:43 +0200 Subject: [PATCH 51/60] CommandFilter API: REDISMODULE_CMDFILTER_NOSELF. Add a flag to automatically protect filters from being called recursively by their own module. --- src/module.c | 28 +++++++++++++++++++++++++--- src/modules/hellofilter.c | 15 +++++++++------ src/redismodule.h | 7 ++++++- tests/modules/commandfilter.tcl | 27 ++++++++++++++++++++++----- 4 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/module.c b/src/module.c index a54bd1ad..4ff865d2 100644 --- a/src/module.c +++ b/src/module.c @@ -50,6 +50,7 @@ struct RedisModule { list *usedby; /* List of modules using APIs from this one. */ list *using; /* List of modules we use some APIs of. */ list *filters; /* List of filters the module has registered. */ + int in_call; /* RM_Call() nesting level */ }; typedef struct RedisModule RedisModule; @@ -283,6 +284,8 @@ typedef struct RedisModuleCommandFilter { RedisModule *module; /* Filter callback function */ RedisModuleCommandFilterFunc callback; + /* REDISMODULE_CMDFILTER_* flags */ + int flags; } RedisModuleCommandFilter; /* Registered filters */ @@ -2756,6 +2759,8 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch c->db = ctx->client->db; c->argv = argv; c->argc = argc; + if (ctx->module) ctx->module->in_call++; + /* We handle the above format error only when the client is setup so that * we can free it normally. */ if (argv == NULL) goto cleanup; @@ -2822,6 +2827,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch autoMemoryAdd(ctx,REDISMODULE_AM_REPLY,reply); cleanup: + if (ctx->module) ctx->module->in_call--; freeClient(c); return reply; } @@ -4857,17 +4863,27 @@ int moduleUnregisterFilters(RedisModule *module) { * * Note that in the above use case, if `MODULE.SET` itself uses * `RedisModule_Call()` the filter will be applied on that call as well. If - * that is not desired, the module itself is responsible for maintaining a flag - * to identify and avoid this form of re-entrancy. + * that is not desired, the `REDISMODULE_CMDFILTER_NOSELF` flag can be set when + * registering the filter. + * + * The `REDISMODULE_CMDFILTER_NOSELF` flag prevents execution flows that + * originate from the module's own `RM_Call()` from reaching the filter. This + * flag is effective for all execution flows, including nested ones, as long as + * the execution begins from the module's command context or a thread-safe + * context that is associated with a blocking command. + * + * Detached thread-safe contexts are *not* associated with the module and cannot + * be protected by this flag. * * If multiple filters are registered (by the same or different modules), they * are executed in the order of registration. */ -RedisModuleCommandFilter *RM_RegisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc callback) { +RedisModuleCommandFilter *RM_RegisterCommandFilter(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc callback, int flags) { RedisModuleCommandFilter *filter = zmalloc(sizeof(*filter)); filter->module = ctx->module; filter->callback = callback; + filter->flags = flags; listAddNodeTail(moduleCommandFilters, filter); listAddNodeTail(ctx->module->filters, filter); @@ -4908,6 +4924,12 @@ void moduleCallCommandFilters(client *c) { while((ln = listNext(&li))) { RedisModuleCommandFilter *f = ln->value; + /* Skip filter if REDISMODULE_CMDFILTER_NOSELF is set and module is + * currently processing a command. + */ + if ((f->flags & REDISMODULE_CMDFILTER_NOSELF) && f->module->in_call) continue; + + /* Call filter */ f->callback(&filter); } diff --git a/src/modules/hellofilter.c b/src/modules/hellofilter.c index 9cd440df..448e1298 100644 --- a/src/modules/hellofilter.c +++ b/src/modules/hellofilter.c @@ -8,7 +8,7 @@ static RedisModuleString *log_key_name; static const char log_command_name[] = "hellofilter.log"; static const char ping_command_name[] = "hellofilter.ping"; static const char unregister_command_name[] = "hellofilter.unregister"; -static int in_module = 0; +static int in_log_command = 0; static RedisModuleCommandFilter *filter = NULL; @@ -57,7 +57,7 @@ int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int ar RedisModule_CloseKey(log); RedisModule_FreeString(ctx, s); - in_module = 1; + in_log_command = 1; size_t cmdlen; const char *cmdname = RedisModule_StringPtrLen(argv[1], &cmdlen); @@ -69,14 +69,14 @@ int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int ar RedisModule_ReplyWithSimpleString(ctx, "Unknown command or invalid arguments"); } - in_module = 0; + in_log_command = 0; return REDISMODULE_OK; } void HelloFilter_CommandFilter(RedisModuleCommandFilterCtx *filter) { - if (in_module) return; /* don't process our own RM_Call() */ + if (in_log_command) return; /* don't process our own RM_Call() from HelloFilter_LogCommand() */ /* Fun manipulations: * - Remove @delme @@ -120,12 +120,14 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if (RedisModule_Init(ctx,"hellofilter",1,REDISMODULE_APIVER_1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if (argc != 1) { + if (argc != 2) { RedisModule_Log(ctx, "warning", "Log key name not specified"); return REDISMODULE_ERR; } + long long noself = 0; log_key_name = RedisModule_CreateStringFromString(ctx, argv[0]); + RedisModule_StringToLongLong(argv[1], &noself); if (RedisModule_CreateCommand(ctx,log_command_name, HelloFilter_LogCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) @@ -139,7 +141,8 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) HelloFilter_UnregisterCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if ((filter = RedisModule_RegisterCommandFilter(ctx, HelloFilter_CommandFilter)) + if ((filter = RedisModule_RegisterCommandFilter(ctx, HelloFilter_CommandFilter, + noself ? REDISMODULE_CMDFILTER_NOSELF : 0)) == NULL) return REDISMODULE_ERR; return REDISMODULE_OK; diff --git a/src/redismodule.h b/src/redismodule.h index 37b7d0d5..e567743a 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -132,6 +132,11 @@ * of timers that are going to expire, sorted by expire time. */ typedef uint64_t RedisModuleTimerID; +/* CommandFilter Flags */ + +/* Do filter RedisModule_Call() commands initiated by module itself. */ +#define REDISMODULE_CMDFILTER_NOSELF (1<<0) + /* ------------------------- End of common defines ------------------------ */ #ifndef REDISMODULE_CORE @@ -340,7 +345,7 @@ void REDISMODULE_API_FUNC(RedisModule_SetDisconnectCallback)(RedisModuleBlockedC void REDISMODULE_API_FUNC(RedisModule_SetClusterFlags)(RedisModuleCtx *ctx, uint64_t flags); int REDISMODULE_API_FUNC(RedisModule_ExportSharedAPI)(RedisModuleCtx *ctx, const char *apiname, void *func); void *REDISMODULE_API_FUNC(RedisModule_GetSharedAPI)(RedisModuleCtx *ctx, const char *apiname); -RedisModuleCommandFilter *REDISMODULE_API_FUNC(RedisModule_RegisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc cb); +RedisModuleCommandFilter *REDISMODULE_API_FUNC(RedisModule_RegisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilterFunc cb, int flags); int REDISMODULE_API_FUNC(RedisModule_UnregisterCommandFilter)(RedisModuleCtx *ctx, RedisModuleCommandFilter *filter); int REDISMODULE_API_FUNC(RedisModule_CommandFilterArgsCount)(RedisModuleCommandFilterCtx *fctx); const RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CommandFilterArgGet)(RedisModuleCommandFilterCtx *fctx, int pos); diff --git a/tests/modules/commandfilter.tcl b/tests/modules/commandfilter.tcl index 8645d827..1e5c41d2 100644 --- a/tests/modules/commandfilter.tcl +++ b/tests/modules/commandfilter.tcl @@ -1,7 +1,7 @@ set testmodule [file normalize src/modules/hellofilter.so] start_server {tags {"modules"}} { - r module load $testmodule log-key + r module load $testmodule log-key 0 test {Command Filter handles redirected commands} { r set mykey @log @@ -50,18 +50,35 @@ start_server {tags {"modules"}} { r lrange log-key 0 -1 } {} - r module load $testmodule log-key-2 + r module load $testmodule log-key 0 test {Command Filter unregister works as expected} { # Validate reloading succeeded + r del log-key r set mykey @log - assert_equal "{set mykey @log}" [r lrange log-key-2 0 -1] + assert_equal "{set mykey @log}" [r lrange log-key 0 -1] # Unregister r hellofilter.unregister - r del log-key-2 + r del log-key r set mykey @log - r lrange log-key-2 0 -1 + r lrange log-key 0 -1 } {} + + r module unload hellofilter + r module load $testmodule log-key 1 + + test {Command Filter REDISMODULE_CMDFILTER_NOSELF works as expected} { + r set mykey @log + assert_equal "{set mykey @log}" [r lrange log-key 0 -1] + + r del log-key + r hellofilter.ping + assert_equal {} [r lrange log-key 0 -1] + + r eval "redis.call('hellofilter.ping')" 0 + assert_equal {} [r lrange log-key 0 -1] + } + } From 29b0a5769576327af24b592f5e2d745fe884c73c Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Tue, 21 Mar 2017 07:20:02 -0700 Subject: [PATCH 52/60] diskless fork kept streaming RDB to a disconnected slave --- src/networking.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/networking.c b/src/networking.c index c08f43e6..09cbff38 100644 --- a/src/networking.c +++ b/src/networking.c @@ -911,6 +911,16 @@ void unlinkClient(client *c) { c->client_list_node = NULL; } + /* In the case of diskless replication the fork is writing to the + * sockets and just closing the fd isn't enough, if we don't also + * shutdown the socket the fork will continue to write to the slave + * and the salve will only find out that it was disconnected when + * it will finish reading the rdb. */ + if ((c->flags & CLIENT_SLAVE) && + (c->replstate == SLAVE_STATE_WAIT_BGSAVE_END)) { + shutdown(c->fd, SHUT_RDWR); + } + /* Unregister async I/O handlers and close the socket. */ aeDeleteFileEvent(server.el,c->fd,AE_READABLE); aeDeleteFileEvent(server.el,c->fd,AE_WRITABLE); From 040e52c77f870be19792b28c73690dbe8b655b9d Mon Sep 17 00:00:00 2001 From: Dvir Volk Date: Thu, 21 Mar 2019 20:33:11 +0200 Subject: [PATCH 53/60] Renamed event name from "miss" to "keymiss" --- src/db.c | 8 ++++---- src/modules/testmodule.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/db.c b/src/db.c index afe18128..b537a29a 100644 --- a/src/db.c +++ b/src/db.c @@ -83,7 +83,7 @@ robj *lookupKey(redisDb *db, robj *key, int flags) { * 1. A key gets expired if it reached it's TTL. * 2. The key last access time is updated. * 3. The global keys hits/misses stats are updated (reported in INFO). - * 4. If keyspace notifications are enabled, a "miss" notification is fired. + * 4. If keyspace notifications are enabled, a "keymiss" notification is fired. * * This API should not be used when we write to the key after obtaining * the object linked to the key, but only for read only operations. @@ -107,7 +107,7 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) { * to return NULL ASAP. */ if (server.masterhost == NULL) { server.stat_keyspace_misses++; - notifyKeyspaceEvent(NOTIFY_KEY_MISS, "miss", key, db->id); + notifyKeyspaceEvent(NOTIFY_KEY_MISS, "keymiss", key, db->id); return NULL; } @@ -129,14 +129,14 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) { server.current_client->cmd->flags & CMD_READONLY) { server.stat_keyspace_misses++; - notifyKeyspaceEvent(NOTIFY_KEY_MISS, "miss", key, db->id); + notifyKeyspaceEvent(NOTIFY_KEY_MISS, "keymiss", key, db->id); return NULL; } } val = lookupKey(db,key,flags); if (val == NULL) { server.stat_keyspace_misses++; - notifyKeyspaceEvent(NOTIFY_KEY_MISS, "miss", key, db->id); + notifyKeyspaceEvent(NOTIFY_KEY_MISS, "keymiss", key, db->id); } else server.stat_keyspace_hits++; diff --git a/src/modules/testmodule.c b/src/modules/testmodule.c index af78d21d..5381380e 100644 --- a/src/modules/testmodule.c +++ b/src/modules/testmodule.c @@ -188,7 +188,7 @@ int TestNotifications(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { RedisModule_Call(ctx, "LPUSH", "cc", "l", "y"); RedisModule_Call(ctx, "LPUSH", "cc", "l", "y"); - /* Miss some keys intentionally so we will get a "miss" notification. */ + /* Miss some keys intentionally so we will get a "keymiss" notification. */ RedisModule_Call(ctx, "GET", "c", "nosuchkey"); RedisModule_Call(ctx, "SMEMBERS", "c", "nosuchkey"); From 822a992f913484162ce508fdb073d8f2ddb6d7c8 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Sun, 24 Mar 2019 12:00:33 +0200 Subject: [PATCH 54/60] fix: missing initialization. --- src/module.c | 1 + src/modules/hellofilter.c => tests/modules/commandfilter.c | 0 tests/{modules => unit/moduleapi}/commandfilter.tcl | 0 3 files changed, 1 insertion(+) rename src/modules/hellofilter.c => tests/modules/commandfilter.c (100%) rename tests/{modules => unit/moduleapi}/commandfilter.tcl (100%) diff --git a/src/module.c b/src/module.c index ff7f27cd..f468d499 100644 --- a/src/module.c +++ b/src/module.c @@ -753,6 +753,7 @@ void RM_SetModuleAttribs(RedisModuleCtx *ctx, const char *name, int ver, int api module->usedby = listCreate(); module->using = listCreate(); module->filters = listCreate(); + module->in_call = 0; ctx->module = module; } diff --git a/src/modules/hellofilter.c b/tests/modules/commandfilter.c similarity index 100% rename from src/modules/hellofilter.c rename to tests/modules/commandfilter.c diff --git a/tests/modules/commandfilter.tcl b/tests/unit/moduleapi/commandfilter.tcl similarity index 100% rename from tests/modules/commandfilter.tcl rename to tests/unit/moduleapi/commandfilter.tcl From ec0b6bd2c35a617101a2e874307be8ae9b504ac0 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Sun, 24 Mar 2019 12:03:03 +0200 Subject: [PATCH 55/60] Add runtest-moduleapi with commandfilter coverage. --- runtest-moduleapi | 16 +++++++++++++++ src/modules/Makefile | 7 +------ tests/modules/Makefile | 24 ++++++++++++++++++++++ tests/modules/commandfilter.c | 28 +++++++++++++------------- tests/test_helper.tcl | 1 - tests/unit/moduleapi/commandfilter.tcl | 16 +++++++-------- 6 files changed, 63 insertions(+), 29 deletions(-) create mode 100755 runtest-moduleapi create mode 100644 tests/modules/Makefile diff --git a/runtest-moduleapi b/runtest-moduleapi new file mode 100755 index 00000000..84cdb9bb --- /dev/null +++ b/runtest-moduleapi @@ -0,0 +1,16 @@ +#!/bin/sh +TCL_VERSIONS="8.5 8.6" +TCLSH="" + +for VERSION in $TCL_VERSIONS; do + TCL=`which tclsh$VERSION 2>/dev/null` && TCLSH=$TCL +done + +if [ -z $TCLSH ] +then + echo "You need tcl 8.5 or newer in order to run the Redis test" + exit 1 +fi + +make -C tests/modules && \ +$TCLSH tests/test_helper.tcl --single unit/moduleapi/commandfilter "${@}" diff --git a/src/modules/Makefile b/src/modules/Makefile index 537aa0da..4f6b50f2 100644 --- a/src/modules/Makefile +++ b/src/modules/Makefile @@ -13,7 +13,7 @@ endif .SUFFIXES: .c .so .xo .o -all: helloworld.so hellotype.so helloblock.so testmodule.so hellocluster.so hellotimer.so hellodict.so hellofilter.so +all: helloworld.so hellotype.so helloblock.so testmodule.so hellocluster.so hellotimer.so hellodict.so .c.xo: $(CC) -I. $(CFLAGS) $(SHOBJ_CFLAGS) -fPIC -c $< -o $@ @@ -47,11 +47,6 @@ hellodict.xo: ../redismodule.h hellodict.so: hellodict.xo -hellofilter.xo: ../redismodule.h - -hellofilter.so: hellofilter.xo - $(LD) -o $@ $< $(SHOBJ_LDFLAGS) $(LIBS) -lc - testmodule.xo: ../redismodule.h testmodule.so: testmodule.xo diff --git a/tests/modules/Makefile b/tests/modules/Makefile new file mode 100644 index 00000000..014d20af --- /dev/null +++ b/tests/modules/Makefile @@ -0,0 +1,24 @@ + +# find the OS +uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not') + +# Compile flags for linux / osx +ifeq ($(uname_S),Linux) + SHOBJ_CFLAGS ?= -W -Wall -fno-common -g -ggdb -std=c99 -O2 + SHOBJ_LDFLAGS ?= -shared +else + SHOBJ_CFLAGS ?= -W -Wall -dynamic -fno-common -g -ggdb -std=c99 -O2 + SHOBJ_LDFLAGS ?= -bundle -undefined dynamic_lookup +endif + +.SUFFIXES: .c .so .xo .o + +all: commandfilter.so + +.c.xo: + $(CC) -I../../src $(CFLAGS) $(SHOBJ_CFLAGS) -fPIC -c $< -o $@ + +commandfilter.xo: ../../src/redismodule.h + +commandfilter.so: commandfilter.xo + $(LD) -o $@ $< $(SHOBJ_LDFLAGS) $(LIBS) -lc diff --git a/tests/modules/commandfilter.c b/tests/modules/commandfilter.c index 448e1298..d25d49c4 100644 --- a/tests/modules/commandfilter.c +++ b/tests/modules/commandfilter.c @@ -1,18 +1,18 @@ #define REDISMODULE_EXPERIMENTAL_API -#include "../redismodule.h" +#include "redismodule.h" #include static RedisModuleString *log_key_name; -static const char log_command_name[] = "hellofilter.log"; -static const char ping_command_name[] = "hellofilter.ping"; -static const char unregister_command_name[] = "hellofilter.unregister"; +static const char log_command_name[] = "commandfilter.log"; +static const char ping_command_name[] = "commandfilter.ping"; +static const char unregister_command_name[] = "commandfilter.unregister"; static int in_log_command = 0; static RedisModuleCommandFilter *filter = NULL; -int HelloFilter_UnregisterCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +int CommandFilter_UnregisterCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { (void) argc; (void) argv; @@ -23,7 +23,7 @@ int HelloFilter_UnregisterCommand(RedisModuleCtx *ctx, RedisModuleString **argv, return REDISMODULE_OK; } -int HelloFilter_PingCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +int CommandFilter_PingCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { (void) argc; (void) argv; @@ -39,7 +39,7 @@ int HelloFilter_PingCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int a return REDISMODULE_OK; } -int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +int CommandFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { RedisModuleString *s = RedisModule_CreateString(ctx, "", 0); @@ -74,9 +74,9 @@ int HelloFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int ar return REDISMODULE_OK; } -void HelloFilter_CommandFilter(RedisModuleCommandFilterCtx *filter) +void CommandFilter_CommandFilter(RedisModuleCommandFilterCtx *filter) { - if (in_log_command) return; /* don't process our own RM_Call() from HelloFilter_LogCommand() */ + if (in_log_command) return; /* don't process our own RM_Call() from CommandFilter_LogCommand() */ /* Fun manipulations: * - Remove @delme @@ -117,7 +117,7 @@ void HelloFilter_CommandFilter(RedisModuleCommandFilterCtx *filter) } int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { - if (RedisModule_Init(ctx,"hellofilter",1,REDISMODULE_APIVER_1) + if (RedisModule_Init(ctx,"commandfilter",1,REDISMODULE_APIVER_1) == REDISMODULE_ERR) return REDISMODULE_ERR; if (argc != 2) { @@ -130,18 +130,18 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) RedisModule_StringToLongLong(argv[1], &noself); if (RedisModule_CreateCommand(ctx,log_command_name, - HelloFilter_LogCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) + CommandFilter_LogCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; if (RedisModule_CreateCommand(ctx,ping_command_name, - HelloFilter_PingCommand,"deny-oom",1,1,1) == REDISMODULE_ERR) + CommandFilter_PingCommand,"deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; if (RedisModule_CreateCommand(ctx,unregister_command_name, - HelloFilter_UnregisterCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) + CommandFilter_UnregisterCommand,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if ((filter = RedisModule_RegisterCommandFilter(ctx, HelloFilter_CommandFilter, + if ((filter = RedisModule_RegisterCommandFilter(ctx, CommandFilter_CommandFilter, noself ? REDISMODULE_CMDFILTER_NOSELF : 0)) == NULL) return REDISMODULE_ERR; diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index d2f28152..568eacde 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -63,7 +63,6 @@ set ::all_tests { unit/lazyfree unit/wait unit/pendingquerybuf - modules/commandfilter } # Index to the next test to run in the ::all_tests list. set ::next_test 0 diff --git a/tests/unit/moduleapi/commandfilter.tcl b/tests/unit/moduleapi/commandfilter.tcl index 1e5c41d2..6078f64f 100644 --- a/tests/unit/moduleapi/commandfilter.tcl +++ b/tests/unit/moduleapi/commandfilter.tcl @@ -1,4 +1,4 @@ -set testmodule [file normalize src/modules/hellofilter.so] +set testmodule [file normalize tests/modules/commandfilter.so] start_server {tags {"modules"}} { r module load $testmodule log-key 0 @@ -27,7 +27,7 @@ start_server {tags {"modules"}} { test {Command Filter applies on RM_Call() commands} { r del log-key - r hellofilter.ping + r commandfilter.ping r lrange log-key 0 -1 } "{ping @log}" @@ -39,13 +39,13 @@ start_server {tags {"modules"}} { test {Command Filter applies on Lua redis.call() that calls a module} { r del log-key - r eval "redis.call('hellofilter.ping')" 0 + r eval "redis.call('commandfilter.ping')" 0 r lrange log-key 0 -1 } "{ping @log}" test {Command Filter is unregistered implicitly on module unload} { r del log-key - r module unload hellofilter + r module unload commandfilter r set mykey @log r lrange log-key 0 -1 } {} @@ -59,14 +59,14 @@ start_server {tags {"modules"}} { assert_equal "{set mykey @log}" [r lrange log-key 0 -1] # Unregister - r hellofilter.unregister + r commandfilter.unregister r del log-key r set mykey @log r lrange log-key 0 -1 } {} - r module unload hellofilter + r module unload commandfilter r module load $testmodule log-key 1 test {Command Filter REDISMODULE_CMDFILTER_NOSELF works as expected} { @@ -74,10 +74,10 @@ start_server {tags {"modules"}} { assert_equal "{set mykey @log}" [r lrange log-key 0 -1] r del log-key - r hellofilter.ping + r commandfilter.ping assert_equal {} [r lrange log-key 0 -1] - r eval "redis.call('hellofilter.ping')" 0 + r eval "redis.call('commandfilter.ping')" 0 assert_equal {} [r lrange log-key 0 -1] } From acba2fc9b4c8082e5344d2d53e51dc4c1c37942c Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 24 Mar 2019 13:10:55 +0200 Subject: [PATCH 56/60] slave corrupts replication stream when module blocked client uses large reply (or POSTPONED_ARRAY) when redis appends the blocked client reply list to the real client, it didn't bother to check if it is in fact the master client. so a slave executing that module command will send replies to the master, causing the master to send the slave error responses, which will mess up the replication offset (slave will advance it's replication offset, and the master does not) --- src/module.c | 7 +------ src/networking.c | 13 +++++++++++++ src/server.h | 1 + 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/module.c b/src/module.c index ff7f27cd..0c8197ac 100644 --- a/src/module.c +++ b/src/module.c @@ -3747,12 +3747,7 @@ void moduleHandleBlockedClients(void) { * We need to glue such replies to the client output buffer and * free the temporary client we just used for the replies. */ if (c) { - if (bc->reply_client->bufpos) - addReplyProto(c,bc->reply_client->buf, - bc->reply_client->bufpos); - if (listLength(bc->reply_client->reply)) - listJoin(c->reply,bc->reply_client->reply); - c->reply_bytes += bc->reply_client->reply_bytes; + AddReplyFromClient(c, bc->reply_client); } freeClient(bc->reply_client); diff --git a/src/networking.c b/src/networking.c index 09cbff38..7fdd1984 100644 --- a/src/networking.c +++ b/src/networking.c @@ -744,6 +744,19 @@ void addReplySubcommandSyntaxError(client *c) { sdsfree(cmd); } +/* Append 'src' client output buffers into 'dst' client output buffers. + * This function clears the output buffers of 'src' */ +void AddReplyFromClient(client *dst, client *src) { + if (prepareClientToWrite(dst) != C_OK) + return; + addReplyProto(dst,src->buf, src->bufpos); + if (listLength(src->reply)) + listJoin(dst->reply,src->reply); + dst->reply_bytes += src->reply_bytes; + src->reply_bytes = 0; + src->bufpos = 0; +} + /* Copy 'src' client output buffers into 'dst' client output buffers. * The function takes care of freeing the old output buffers of the * destination client. */ diff --git a/src/server.h b/src/server.h index 95e0355a..dfd9f769 100644 --- a/src/server.h +++ b/src/server.h @@ -1529,6 +1529,7 @@ void addReplyNullArray(client *c); void addReplyBool(client *c, int b); void addReplyVerbatim(client *c, const char *s, size_t len, const char *ext); void addReplyProto(client *c, const char *s, size_t len); +void AddReplyFromClient(client *c, client *src); void addReplyBulk(client *c, robj *obj); void addReplyBulkCString(client *c, const char *s); void addReplyBulkCBuffer(client *c, const void *p, size_t len); From 75648f99a5ba41812c115f83f8b668f030acfaee Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 8 Apr 2019 17:39:22 +0200 Subject: [PATCH 57/60] Fix assert comparison in fetchClusterSlotsConfiguration(). --- src/redis-benchmark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/redis-benchmark.c b/src/redis-benchmark.c index 12e9f7e4..4e2662f2 100644 --- a/src/redis-benchmark.c +++ b/src/redis-benchmark.c @@ -1192,7 +1192,7 @@ static int fetchClusterSlotsConfiguration(client c) { assert(reply->type == REDIS_REPLY_ARRAY); for (i = 0; i < reply->elements; i++) { redisReply *r = reply->element[i]; - assert(r->type = REDIS_REPLY_ARRAY); + assert(r->type == REDIS_REPLY_ARRAY); assert(r->elements >= 3); int from, to, slot; from = r->element[0]->integer; From f8a9708aa705b6493ef63a82e42ed428997b817a Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 8 Apr 2019 18:06:50 +0200 Subject: [PATCH 58/60] ACL: regression test for #5998. --- tests/unit/acl.tcl | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 82c75f82..90f2c9bb 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -108,4 +108,11 @@ start_server {tags {"acl"}} { assert_match {*+debug|segfault*} $cmdstr assert_match {*+acl*} $cmdstr } + + test {ACL regression: memory leaks adding / removing subcommands} { + r AUTH default "" + r ACL setuser newuser reset -debug +debug|a +debug|b +debug|c + r ACL setuser newuser -debug + # The test framework will detect a leak if any. + } } From c24e32041b91ac32626e8d8eee1c062942e25f27 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 8 Apr 2019 18:08:37 +0200 Subject: [PATCH 59/60] ACL: Fix memory leak in ACLResetSubcommandsForCommand(). This commit fixes bug reported at #5998. Thanks to @tomcat1102. --- src/acl.c | 2 ++ tests/unit/acl.tcl | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/acl.c b/src/acl.c index d9f431f4..0205e51a 100644 --- a/src/acl.c +++ b/src/acl.c @@ -542,6 +542,8 @@ struct redisCommand *ACLLookupCommand(const char *name) { * and command ID. */ void ACLResetSubcommandsForCommand(user *u, unsigned long id) { if (u->allowed_subcommands && u->allowed_subcommands[id]) { + for (int i = 0; u->allowed_subcommands[id][i]; i++) + sdsfree(u->allowed_subcommands[id][i]); zfree(u->allowed_subcommands[id]); u->allowed_subcommands[id] = NULL; } diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 90f2c9bb..05844143 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -109,7 +109,7 @@ start_server {tags {"acl"}} { assert_match {*+acl*} $cmdstr } - test {ACL regression: memory leaks adding / removing subcommands} { + test {ACL #5998 regression: memory leaks adding / removing subcommands} { r AUTH default "" r ACL setuser newuser reset -debug +debug|a +debug|b +debug|c r ACL setuser newuser -debug From 9e67691ffb4709535b56a089a973c3f89bfbe231 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 10 Apr 2019 18:53:27 +0200 Subject: [PATCH 60/60] Aesthetic change to #5962 to conform to Redis style. --- src/module.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/module.c b/src/module.c index 0c8197ac..1e7c0eca 100644 --- a/src/module.c +++ b/src/module.c @@ -3746,9 +3746,7 @@ void moduleHandleBlockedClients(void) { * replies to send to the client in a thread safe context. * We need to glue such replies to the client output buffer and * free the temporary client we just used for the replies. */ - if (c) { - AddReplyFromClient(c, bc->reply_client); - } + if (c) AddReplyFromClient(c, bc->reply_client); freeClient(bc->reply_client); if (c != NULL) {