On Wed, Dec 03, 2014 at 09:47:08PM +0100, Lukas Slebodnik wrote:
On (03/12/14 18:27), Nikolai Kondrashov wrote:
>On 12/03/2014 06:19 PM, Nikolai Kondrashov wrote:
>>Hi everyone,
>>
>>The attached patch removes Valgrind leak checking as suggested by Lukas on
>>private sssd-ci list:
>>
>> Valgrind will ***NEVER*** find memory leaks in sssd. We use talloc for
memory
>> allocations and maintaining suppresion database is nightmare from my POV.
>>
>> I vote for removing testing memory leaks with valgrind
>>
>>Personally, I'm not sure if leak checking is completely useless, but perhaps
>>it is. Has anyone caught any leaks with this so far?
>>
>>Regarding maintenance, we haven't had to add any new leak suppressions in
more
>>than two months, so it doesn't look so bad so far.
>>
>>However, if we feel this is irritating and/or useless, we should remove it.
>>
>>Does anyone have an opinion on this?
>
>Oh, and Jakub has said this in response to Lukas:
>
> We should keep the valgrind tests for checking for unallocated access or
> use-after free.
>
> Checking for leaks would only make sense if we compiled our external
> libraries (like the idmap library) with malloc instead of talloc.
>
My opinion is clean. Jakub would like to have checking memory leaks
with valgrind.
I thought about the leaks some more. There /are/ cases where leak checks
are valuable even though we use talloc -- one example is the tests Sumit
pushed the other day. Some of the Kerberos functions internally allocate
data and must be freed with other corresponding libkrb5 functions.
Given that Sumit also sent a patch that fixed a leak in leak checking
code (sic!) I do think developers care about valgrind
Therefore, I think it still makes sense to keep the leak checks around.
We need to find a compromise.
Therefore NACK.
BTW.
a) This is a reason why maintaining suppression database in git tree is the
worst solution. (it would be even worse with clang static analyser)
b) Our CI should be more flexible. Currently we need to wait until solution
will be pushed to the upstream. Meanwhile, CI is unusable.
I think this is a valid point.
Well, could we keep the suppression databases on the CI server itself? Maybe
in some 'staging' branch that would eventually be merged upstream if we
absolutely need to keep the suppression upstream?
Or could we have some directory on the CI where we would just drop a
file while a patch waits for a review?
There should be simple way how to override settings for specific plaform
or temporary disable test and do not touch referential implementation it git
tree. The problem should be fixed immediately (in ideal situation).
It didn't happen with libselinux valgrind problem. As a result of this new
issues were introduced. If there was a way how to temporary disable valgrind
test on older platforms we would faster find issue with old kerberos function
on rhel6 and problems with libpopt.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel