[SSSD] [PATCH v3] CI: Enforce Valgrind check

Jakub Hrozek jhrozek at redhat.com
Thu Sep 18 16:21:19 UTC 2014


On Thu, Sep 18, 2014 at 04:22:12PM +0200, Lukas Slebodnik wrote:
> On (18/09/14 10:11), Nikolai Kondrashov wrote:
> >On 09/17/2014 07:48 PM, Lukas Slebodnik wrote:
> >>On (17/09/14 18:48), Nikolai Kondrashov wrote:
> >>>+	$(NULL)
> >>  ^^^
> >>  '\t' instead of spaces
> >>Please follow our conventions.
> >
> >Fixed, thank you.
> >
> >>>         stage make-check-valgrind \
> >>>                 make-check-wrap -j $CPU_NUM check -- \
> >>>                                 libtool --mode=execute \
> >>>-                                    valgrind-condense 99 '!(*.py)' -- \
> >>>+                                    valgrind-condense 99 \
> >>>+                                    '!(*.py|*dlopen-tests)' -- \
> >>>                                     --vgdb=no \
> >>                                       ^^^^^^^^^
> >>                     Why do we need to have disabled vgdb?
> >>It is true that vgdb is enabled by default, but default value of vgdb-error is
> >>999999999.
> >
> >Yeah, it is almost impossible it will be hit. And vgdb is enabled to make it
> >easier to enable it by lowering vgdb-error. But I'd prefer not to care about
> >CI hanging, waiting on input for whatever reason.
> >
> I would prefer to remove it. It's easier to understand small code.
> and tere are lot of other necessary options wich cannot be removed.

Sounds like a fair point and an easy fix..

> 
> >>>                                     --trace-children=yes \
> >>>                                     --trace-children-skip='*/bin/*,*/sbin/*' \
> >>
> >>Could you explain your style of indentation here?
> >>I cannot find any rule for indentation. Somewhere are 4 spaces, elsewhere 8 ...
> >>As a result of this unusual indentation  almost everything is near the 80
> >>column limit.
> >
> >I try to place arguments past the end of their command's name, or at least
> >past the start, to make it more visible. In case the arguments don't fit, I
> >move them to the left.
> >
> >The command above is pretty nested, that's why the arguments are so far to the
> >right.
> >
> >>----------------
> >>
> >>You are mixing pates related to ticket #2428 with others
> >>    1st) contains: "Related to https://fedorahosted.org/sssd/ticket/2428"
> >>    3rd) contains: "Fixes https://fedorahosted.org/sssd/ticket/2428"
> >>    2nd, 4th) does not contain anything
> >
> Could you at least change the order of patches?
> 1st: 0002
> 2nd: 0001
> 3rd: 0003
> 4th: 0004
> 
> >Yes. Because the ticket subject says "Prepare suppression database for
> >valgrind test" and nothing about enforcing Valgrind check.
> >
> >>It seems that you don't use our git commit template
> >>@see .git-commit-template
> >>@see git show 3d9edb4c510028def2df41aa7b0ce705b197e6fc
> >
> >Fixed, thank you.
> >
> >Nick
> 
> Almost ACK, but I still have few other small comments.
> I apologize that I didn't notice them in previous rounds.
> I hope issues will be solved very fast and we can finally rely on valgrind
> tests.
> 

[...]

> >+# Ignore tests exiting and abandoning cmocka state, concerns dyndns test
> >+{
> >+   sssd-leak-cmocka-exit
> >+   Memcheck:Leak
> >+   fun:malloc
> >+   fun:_test_malloc
> >+   fun:_run_tests
> >+   fun:main
> >+}
> >+
> >+# Stpncpy bug or false positive on RHEL6, likely involving SSE
>    This explanation is vague. I would appreciate long explanation (even more
>    than 30 lines) or link to archive, wiki, stackoverflow ...
>    So, anyone can find the reason.

If there is a link, let's include it but let's not write "30 lines"
about a bug on a stable platform. A link would be best, if there's none,
let's go with what we have.

> >+{
> >+   sssd-value8-stpncpy
> >+   Memcheck:Value8
> >+   obj:/lib*/libc-2.12.so
> >+   fun:sha512_crypt_r
> >+   ...
> >+}
> >+{
> >+   sssd-cond-stpncpy
> >+   Memcheck:Cond
> >+   obj:/lib*/libc-2.12.so
> >+   fun:sha512_crypt_r
> >+   ...
> >+}
> >+
> >+# False positive leak involving RHEL6 glib memory slices
> >+{
> >+   sssd-leak-glib-slices
> >+   Memcheck:Leak
> >+   fun:memalign
> >+   fun:posix_memalign
> >+   obj:/lib*/libglib-2.0.so*
> >+   fun:g_slice_alloc
> >+   fun:g_string_sized_new
> >+   fun:g_utf8_casefold
> >+   ...
> >+}
> >-- 
> >2.1.0
> >
> 
> >From f6867568e3e5ea985eab8aa75f5b1681957458e9 Mon Sep 17 00:00:00 2001
> >From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
> >Date: Wed, 17 Sep 2014 15:37:41 +0300
> >Subject: [PATCH 4/4] CI: Enforce Valgrind check
> >
> >Add check for Valgrind test result to contrib/ci/run.
> >---
> > contrib/ci/run | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/contrib/ci/run b/contrib/ci/run
> >index 68ec1aa..a960319 100755
> >--- a/contrib/ci/run
> >+++ b/contrib/ci/run
> >@@ -197,7 +197,6 @@ function build_debug()
> >     # Not building "tests" due to https://fedorahosted.org/sssd/ticket/2350
> >     stage make-tests        make-check-wrap -j $CPU_NUM check -- true
> > 
> >-    # Ignored until issues found by Valgrind are fixed
> >     status=0
> >     CK_FORK=no \
> >         stage make-check-valgrind \
> >@@ -214,6 +213,7 @@ function build_debug()
> >                                     --verbose ||
> >             status=$?
> >     mv "$test_dir" ci-test-dir
> >+    ((status == 0))
> 
> Sorry, I still don't understan your kind of indentation.
> Here is the patrt of code:
> 200     status=0
> 201     CK_FORK=no \
> 202         stage make-check-valgrind \
> 203                 make-check-wrap -j $CPU_NUM check -- \
> 204                                 libtool --mode=execute \
> 205                                     valgrind-condense 99 \
> 206                                     '!(*.py|*dlopen-tests)' -- \
> 207                                     --vgdb=no \
> 208                                     --trace-children=yes \
> 209                                     --trace-children-skip='*/bin/*,*/sbin/*' \
> 210                                     --leak-check=full \
> 211                                     --gen-suppressions=all \
> 212                                     --suppressions="$CI_DIR/sssd.supp" \
> 213                                     --verbose ||
> 214             status=$?
> 215     mv "$test_dir" ci-test-dir
> 216     ((status == 0))
> 
> I can understand that libtool on line 204 is alligned o the "-j $CPU_NUM"
> ans next lines are indented with additional 4 spaces.
> 
> Why are there 8 additional spaces on line 203?
> Line 214: What is variable status alligned to?

The comment about status and line 203 seems valid.

Let's fix these nitpicks and push the patches. Enough time was spend
reviewing it already.



More information about the sssd-devel mailing list