Binary safe strings #538
Binary safe strings #538
Conversation
lantiga
changed the title
lantiga
force-pushed
the
binary-safe-strings
branch
from
c28e165 to
02e6c01
Compare
Thank you Alon. That's interesting, I just read the docstring for HoldString. I'll make the change, thanks. |
|
@alonre24 done with the HoldString change |
src/util/dict.c
Outdated
| } | ||
|
|
||
| static void *rstringsKeyDup(void *privdata, const void *key) { | ||
| return RedisModule_CreateStringFromString(NULL, (RedisModuleString *)key); |
There was a problem hiding this comment.
maybe RAI_HoldString to avoid strdup
There was a problem hiding this comment.
I'd avoid this as it could lead to inter-thread issues (in fact I'm getting a segfault). Need to track if further, let's keep it as is for this PR.
src/run_info.c
Outdated
|
|
||
| static void *RAI_TensorDictKeyDup(void *privdata, const void *key) { | ||
| return RedisModule_Strdup((char *)key); | ||
| return RedisModule_CreateStringFromString(NULL, (RedisModuleString *)key); |
There was a problem hiding this comment.
It's now the same code as above, so same consideration.
src/run_info.c
Outdated
| size_t len; | ||
| const char *buffer = RedisModule_StringPtrLen((RedisModuleString *)key, &len); | ||
| return AI_dictGenHashFunction(buffer, len); | ||
| } | ||
|
|
||
| static int RAI_TensorDictKeyStrcmp(void *privdata, const void *key1, const void *key2) { | ||
| const char *strKey1 = key1; | ||
| const char *strKey2 = key2; | ||
| return strcmp(strKey1, strKey2) == 0; | ||
| RedisModuleString *strKey1 = (RedisModuleString *)key1; | ||
| RedisModuleString *strKey2 = (RedisModuleString *)key2; | ||
| return RedisModule_StringCompare(strKey1, strKey2) == 0; | ||
| } | ||
|
|
||
| static void RAI_TensorDictKeyFree(void *privdata, void *key) { RedisModule_Free(key); } | ||
| static void RAI_TensorDictKeyFree(void *privdata, void *key) { | ||
| RedisModule_FreeString(NULL, (RedisModuleString *)key); | ||
| } | ||
|
|
||
| static void *RAI_TensorDictKeyDup(void *privdata, const void *key) { | ||
| return RedisModule_Strdup((char *)key); | ||
| return RedisModule_CreateStringFromString(NULL, (RedisModuleString *)key); |
There was a problem hiding this comment.
This is duplicated logic of the RSring dictionaries. Any chance to consolidate?
| #include <string.h> | ||
| #include "../redisai_memory.h" | ||
|
|
||
| RedisModuleString *RAI_HoldString(RedisModuleCtx *ctx, RedisModuleString *str) { |
There was a problem hiding this comment.
This util is a good idea! But the first argument is redundant (we always send NULL as ctx).
When I come to think of it, if we know that we won't use the auto_memory mechanism, we may consider adding a util function such as "RAI_FreeString(RedisModuleString *str)", and replace every call for RedisModule_FreeString with that - so we avoid the annoying bugs. This can probably be done in another PR...
Codecov Report
@@ Coverage Diff @@
## master #538 +/- ##
==========================================
+ Coverage 75.31% 75.33% +0.01%
==========================================
Files 22 22
Lines 5226 5221 -5
==========================================
- Hits 3936 3933 -3
+ Misses 1290 1288 -2
Continue to review full report at Codecov.
|
|
Yes! 🎉 Thank you @alonre24 for spotting the bug that was making |
Address #508 by converting all use of user-supplied strings (except device names, TF node names and script source) to binary-safe strings (RedisModuleString).