Adds assertions in RedisModule_Init when RedisModule_GetApi return ERR#11033
Adds assertions in RedisModule_Init when RedisModule_GetApi return ERR#11033enjoy-binbin wants to merge 3 commits intoredis:unstablefrom
Conversation
Prevent forgetting to register API, adding assertions, see redis#11025 Also remove `ReplySetPushLength`, it is a dead code.
src/redismodule.h
Outdated
|
|
||
| #define REDISMODULE_GET_API(name) \ | ||
| RedisModule_GetApi("RedisModule_" #name, ((void **)&RedisModule_ ## name)) | ||
| assert(RedisModule_GetApi("RedisModule_" #name, ((void **)&RedisModule_ ## name)) == REDISMODULE_OK) |
There was a problem hiding this comment.
for a moment i thought that we might have issues with NDEBUG if the caller defined it.
but these lines are actually compiled as part of redis binary.
however, maybe we still rather just set a boolean and handle it at the end of RedisModule_Init?
arguably a static assertion would have been sufficient if we had such an option, and i imagine that adding an assertion per line would blow up the executable size a bit...
There was a problem hiding this comment.
i ended up using a static REDISMODULE_GET_API_RET, and using RedisModule_Assert handle it at the end of RedisModule_Init. using RedisModule_Assert so that we can print out the error log
| REDISMODULE_API int (*RedisModule_LoadConfigs)(RedisModuleCtx *ctx) REDISMODULE_ATTR; | ||
|
|
||
| #define RedisModule_IsAOFClient(id) ((id) == UINT64_MAX) | ||
| #define RedisModule_Assert(_e) ((_e)?(void)0 : (RedisModule__Assert(#_e,__FILE__,__LINE__),exit(1))) |
There was a problem hiding this comment.
this change because:
[root@binblog redis]# ./runtest-moduleapi
make: Entering directory '/root/redis/self/redis/tests/modules'
gcc -I../../src -W -Wall -Wno-missing-field-initializers -fno-common -g -ggdb -std=c99 -O2 -fPIC -c commandfilter.c -o commandfilter.xo
In file included from commandfilter.c:1:0:
../../src/redismodule.h: In function ‘RedisModule_Init’:
../../src/redismodule.h:1545:5: warning: implicit declaration of function ‘RedisModule_Assert’; did you mean ‘RedisModule__Assert’? [-Wimplicit-function-declaration]
RedisModule_Assert(REDISMODULE_GET_API_RET);
^~~~~~~~~~~~~~~~~~
RedisModule__Assert
|
I don't know what i was drinking when i proposed this solution, but i now realize it'll mean modules that are built with a new header file will be unable to load into an old redis server. |
oops, that's right, so we cannot do it |
|
the only solution i can think of is some module test that parses the header file, and for each API it finds declared, it calls RedisModule_GetApi to make sure it was registered. |
2 participants
Prevent forgetting to register API, adding assertions, see #11025
Also remove
ReplySetPushLength, it is a dead code.