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
