From 406caa5f569407664ce4d1f3203680fc1e930b1c Mon Sep 17 00:00:00 2001 From: Evan Date: Tue, 22 Jun 2021 05:26:48 -0400 Subject: [PATCH] modules: Add newlen == 0 handling to RM_StringTruncate (#3717) (#3718) Previously, passing 0 for newlen would not truncate the string at all. This adds handling of this case, freeing the old string and creating a new empty string. Other changes: - Move `src/modules/testmodule.c` to `tests/modules/basics.c` - Introduce that basic test into the test suite - Add tests to cover StringTruncate - Add `test-modules` build target for the main makefile - Extend `distclean` build target to clean modules too (cherry picked from commit 1ccf2ca2f475a161b8ca0c574d4c0e6ef9ecf754) --- runtest-moduleapi | 1 + src/Makefile | 5 ++ src/module.c | 21 +++-- tests/modules/Makefile | 1 + .../testmodule.c => tests/modules/basics.c | 85 +++++++++++++++++-- tests/unit/moduleapi/basics.tcl | 13 +++ 6 files changed, 110 insertions(+), 16 deletions(-) rename src/modules/testmodule.c => tests/modules/basics.c (83%) create mode 100644 tests/unit/moduleapi/basics.tcl diff --git a/runtest-moduleapi b/runtest-moduleapi index 7c17501e..56d14960 100755 --- a/runtest-moduleapi +++ b/runtest-moduleapi @@ -16,6 +16,7 @@ fi $MAKE -C tests/modules && \ $TCLSH tests/test_helper.tcl \ --single unit/moduleapi/commandfilter \ +--single unit/moduleapi/basics \ --single unit/moduleapi/fork \ --single unit/moduleapi/testrdb \ --single unit/moduleapi/infotest \ diff --git a/src/Makefile b/src/Makefile index 62e37cb4..cf3e8c03 100644 --- a/src/Makefile +++ b/src/Makefile @@ -375,6 +375,8 @@ clean: distclean: clean -(cd ../deps && $(MAKE) distclean) + -(cd modules && $(MAKE) clean) + -(cd ../tests/modules && $(MAKE) clean) -(rm -f .make-*) .PHONY: distclean @@ -382,6 +384,9 @@ distclean: clean test: $(REDIS_SERVER_NAME) $(REDIS_CHECK_AOF_NAME) $(REDIS_CLI_NAME) $(REDIS_BENCHMARK_NAME) @(cd ..; ./runtest) +test-modules: $(REDIS_SERVER_NAME) + @(cd ..; ./runtest-moduleapi) + test-sentinel: $(REDIS_SENTINEL_NAME) $(REDIS_CLI_NAME) @(cd ..; ./runtest-sentinel) diff --git a/src/module.c b/src/module.c index df28bc8c..720befe2 100644 --- a/src/module.c +++ b/src/module.c @@ -2538,14 +2538,19 @@ int RM_StringTruncate(RedisModuleKey *key, size_t newlen) { } else { /* Unshare and resize. */ key->value = dbUnshareStringValue(key->db, key->key, key->value); - size_t curlen = sdslen(key->value->ptr); - if (newlen > curlen) { - key->value->ptr = sdsgrowzero(key->value->ptr,newlen); - } else if (newlen < curlen) { - sdsrange(key->value->ptr,0,newlen-1); - /* If the string is too wasteful, reallocate it. */ - if (sdslen(key->value->ptr) < sdsavail(key->value->ptr)) - key->value->ptr = sdsRemoveFreeSpace(key->value->ptr); + if (newlen == 0) { + sdsfree(key->value->ptr); + key->value->ptr = sdsempty(); + } else { + size_t curlen = sdslen(key->value->ptr); + if (newlen > curlen) { + key->value->ptr = sdsgrowzero(key->value->ptr,newlen); + } else if (newlen < curlen) { + sdsrange(key->value->ptr,0,newlen-1); + /* If the string is too wasteful, reallocate it. */ + if (sdslen(key->value->ptr) < sdsavail(key->value->ptr)) + key->value->ptr = sdsRemoveFreeSpace(key->value->ptr); + } } } return REDISMODULE_OK; diff --git a/tests/modules/Makefile b/tests/modules/Makefile index f5631396..ae611de8 100644 --- a/tests/modules/Makefile +++ b/tests/modules/Makefile @@ -18,6 +18,7 @@ endif TEST_MODULES = \ commandfilter.so \ + basics.so \ testrdb.so \ fork.so \ infotest.so \ diff --git a/src/modules/testmodule.c b/tests/modules/basics.c similarity index 83% rename from src/modules/testmodule.c rename to tests/modules/basics.c index 078c02c5..9786be1b 100644 --- a/src/modules/testmodule.c +++ b/tests/modules/basics.c @@ -31,7 +31,7 @@ */ #define REDISMODULE_EXPERIMENTAL_API -#include "../redismodule.h" +#include "redismodule.h" #include /* --------------------------------- Helpers -------------------------------- */ @@ -152,7 +152,58 @@ int TestUnlink(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { return failTest(ctx, "Could not verify key to be unlinked"); } return RedisModule_ReplyWithSimpleString(ctx, "OK"); +} +/* TEST.STRING.TRUNCATE -- Test truncating an existing string object. */ +int TestStringTruncate(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + REDISMODULE_NOT_USED(argv); + REDISMODULE_NOT_USED(argc); + + RedisModule_Call(ctx, "SET", "cc", "foo", "abcde"); + RedisModuleKey *k = RedisModule_OpenKey(ctx, RedisModule_CreateStringPrintf(ctx, "foo"), REDISMODULE_READ | REDISMODULE_WRITE); + if (!k) return failTest(ctx, "Could not create key"); + + size_t len = 0; + char* s; + + /* expand from 5 to 8 and check null pad */ + if (REDISMODULE_ERR == RedisModule_StringTruncate(k, 8)) { + return failTest(ctx, "Could not truncate string value (8)"); + } + s = RedisModule_StringDMA(k, &len, REDISMODULE_READ); + if (!s) { + return failTest(ctx, "Failed to read truncated string (8)"); + } else if (len != 8) { + return failTest(ctx, "Failed to expand string value (8)"); + } else if (0 != strncmp(s, "abcde\0\0\0", 8)) { + return failTest(ctx, "Failed to null pad string value (8)"); + } + + /* shrink from 8 to 4 */ + if (REDISMODULE_ERR == RedisModule_StringTruncate(k, 4)) { + return failTest(ctx, "Could not truncate string value (4)"); + } + s = RedisModule_StringDMA(k, &len, REDISMODULE_READ); + if (!s) { + return failTest(ctx, "Failed to read truncated string (4)"); + } else if (len != 4) { + return failTest(ctx, "Failed to shrink string value (4)"); + } else if (0 != strncmp(s, "abcd", 4)) { + return failTest(ctx, "Failed to truncate string value (4)"); + } + + /* shrink to 0 */ + if (REDISMODULE_ERR == RedisModule_StringTruncate(k, 0)) { + return failTest(ctx, "Could not truncate string value (0)"); + } + s = RedisModule_StringDMA(k, &len, REDISMODULE_READ); + if (!s) { + return failTest(ctx, "Failed to read truncated string (0)"); + } else if (len != 0) { + return failTest(ctx, "Failed to shrink string value to (0)"); + } + + return RedisModule_ReplyWithSimpleString(ctx, "OK"); } int NotifyCallback(RedisModuleCtx *ctx, int type, const char *event, @@ -324,7 +375,11 @@ end: int TestAssertStringReply(RedisModuleCtx *ctx, RedisModuleCallReply *reply, char *str, size_t len) { RedisModuleString *mystr, *expected; - if (RedisModule_CallReplyType(reply) != REDISMODULE_REPLY_STRING) { + if (RedisModule_CallReplyType(reply) == REDISMODULE_REPLY_ERROR) { + RedisModule_Log(ctx,"warning","Test error reply: %s", + RedisModule_CallReplyStringPtr(reply, NULL)); + return 0; + } else if (RedisModule_CallReplyType(reply) != REDISMODULE_REPLY_STRING) { RedisModule_Log(ctx,"warning","Unexpected reply type %d", RedisModule_CallReplyType(reply)); return 0; @@ -345,7 +400,11 @@ int TestAssertStringReply(RedisModuleCtx *ctx, RedisModuleCallReply *reply, char /* Return 1 if the reply matches the specified integer, otherwise log errors * in the server log and return 0. */ int TestAssertIntegerReply(RedisModuleCtx *ctx, RedisModuleCallReply *reply, long long expected) { - if (RedisModule_CallReplyType(reply) != REDISMODULE_REPLY_INTEGER) { + if (RedisModule_CallReplyType(reply) == REDISMODULE_REPLY_ERROR) { + RedisModule_Log(ctx,"warning","Test error reply: %s", + RedisModule_CallReplyStringPtr(reply, NULL)); + return 0; + } else if (RedisModule_CallReplyType(reply) != REDISMODULE_REPLY_INTEGER) { RedisModule_Log(ctx,"warning","Unexpected reply type %d", RedisModule_CallReplyType(reply)); return 0; @@ -366,8 +425,11 @@ int TestAssertIntegerReply(RedisModuleCtx *ctx, RedisModuleCallReply *reply, lon reply = RedisModule_Call(ctx,name,__VA_ARGS__); \ } while (0) -/* TEST.IT -- Run all the tests. */ -int TestIt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { +/* TEST.BASICS -- Run all the tests. + * Note: it is useful to run these tests from the module rather than TCL + * since it's easier to check the reply types like that (make a distinction + * between 0 and "0", etc. */ +int TestBasics(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { REDISMODULE_NOT_USED(argv); REDISMODULE_NOT_USED(argc); @@ -390,6 +452,9 @@ int TestIt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { T("test.string.append",""); if (!TestAssertStringReply(ctx,reply,"foobar",6)) goto fail; + T("test.string.truncate",""); + if (!TestAssertStringReply(ctx,reply,"OK",2)) goto fail; + T("test.unlink",""); if (!TestAssertStringReply(ctx,reply,"OK",2)) goto fail; @@ -407,7 +472,7 @@ int TestIt(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { fail: RedisModule_ReplyWithSimpleString(ctx, - "SOME TEST NOT PASSED! Check server logs"); + "SOME TEST DID NOT PASS! Check server logs"); return REDISMODULE_OK; } @@ -430,6 +495,10 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) TestStringAppendAM,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,"test.string.truncate", + TestStringTruncate,"write deny-oom",1,1,1) == REDISMODULE_ERR) + return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,"test.string.printf", TestStringPrintf,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; @@ -442,8 +511,8 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) TestUnlink,"write deny-oom",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; - if (RedisModule_CreateCommand(ctx,"test.it", - TestIt,"readonly",1,1,1) == REDISMODULE_ERR) + if (RedisModule_CreateCommand(ctx,"test.basics", + TestBasics,"readonly",1,1,1) == REDISMODULE_ERR) return REDISMODULE_ERR; RedisModule_SubscribeToKeyspaceEvents(ctx, diff --git a/tests/unit/moduleapi/basics.tcl b/tests/unit/moduleapi/basics.tcl new file mode 100644 index 00000000..513ace6b --- /dev/null +++ b/tests/unit/moduleapi/basics.tcl @@ -0,0 +1,13 @@ +set testmodule [file normalize tests/modules/basics.so] + + +# TEST.CTXFLAGS requires RDB to be disabled, so override save file +start_server {tags {"modules"} overrides {save ""}} { + r module load $testmodule + + test {test module api basics} { + r test.basics + } {ALL TESTS PASSED} + + r module unload test +}