[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