Okay, thanks.
Will try that and get the output
Duncan
> On 10 Mar 2021, at 19:55, Wietse Venema <wie...@porcupine.org> wrote:
>
> Dunk:
>> Hi,
>> Please see latest version of the patch, I tried testing with valgrind under
>> Alpine Linux in a Docker container and it didn?t report any errors. But a
>> ?postfix stop? exits postfix/valgrind without a summary.
>>
>> Duncan
>
> Valgrind always produces output. If you don't get any then your
> test is invalid.
>
> Can you test this with:
>
> $ cd src/global
> $ make mail_dict
> cd ../..
> $ sh postfix-env.sh valgrind --tool=memcheck src/global/mail_dict
> redis:/path/to/file read <<'EOF'
> get something-that-exists
> get something-that-does-not-exist
> EOF
>
> I will wait for real valgrind output before I look at the code.
>
> Wietse
>
>>
>>>> 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
