Thanks for the feedback, I will make the changes and test with VALGRIND.
Duncan
> On 27 Feb 2021, at 23:20, Wietse Venema <wie...@porcupine.org> wrote:
>
> Duncan Bellamy:
>> Hi,
>>
>> This patch is based on the original code by Titus Jose on GitHub, I
>> updated it to work with the development branch and have added some
>> documentation.
>
> Thanks. I have a few suggestions regarding memory leaks, data
> owbership, and testing.
>
> 115 static const char *dict_redis_lookup(DICT *dict, const char *name)
> 116 {
> ...
> 124 result = vstring_alloc(10);
> 125 VSTRING_RESET(result); /* Not needed -- WZV */
> 126 VSTRING_TERMINATE(result); /* Not needed -- WZV */
>
> I think that you're leaking one VSTRING per dict_redis_lookup()
> call. With the Postfix DICT variants, a dictionary owns the lookup
> result (no multi-threading), it is therefore OK to make the result
> a member of the DICT_REDIS instance.
>
> 141 if(dict_redis->c) {
> 142 reply = redisCommand(dict_redis->c,"GET %s%s",dict_redis->prefix
> 142 ,name);
> 143 }
> 144 else { /* Can't happen - WZV */
> 145 dict->error = DICT_ERR_CONFIG;
> 146 }
>
> The code in dict_redis_open guarantees that a DICT_REDIS instance
> never has null DICT_REDIS.c member. You can delete the test and the
> unreachable branch.
>
> 172 DICT *dict_redis_open(const char *name, int open_flags, int dict_flags
> 172 )
> 173 {
> ...
> 193 c = redisConnect(dict_redis->host,dict_redis->port);
> 194 if(c->err) { /* Redirect to surrogate -- WZV */
> 195 msg_fatal("%s:%s: Cannot connect to Redis server %s: %s\n",
> 196 DICT_TYPE_REDIS, name, dict_redis->host, c->errstr);
>
> For consistency with other code, this should
>
> 1) Free the 'c'
> 2) Set dict_redis->c to null and invoke dict_close(&dict_redis)
> 3) Return dict_surrogate(DICT_TYPE_REDIS, name, ...)
>
> 206 static void dict_redis_close(DICT *dict)
> 207 {
> 208 DICT_REDIS *dict_redis = (DICT_REDIS *) dict;
>
> This function leaks the dict_redis->c pointer.
> Here, call free dict_redis->c if it is not null.
>
> Be sure to test this under VALGRIND. See Makefile
> for how to build and run mail_dict.c.
>
> Wietse
