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

Lukas Slebodnik lslebodn at redhat.com
Thu Sep 18 14:22:12 UTC 2014


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.

>>>                                     --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.

>From 9ee48b20d819d6bbc6aa54295ccd62b9be95c7ed Mon Sep 17 00:00:00 2001
>From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>Date: Thu, 4 Sep 2014 14:04:15 +0300
>Subject: [PATCH 1/4] CI: Add Valgrind suppression support
>
>Add an empty Valgrind suppressions file, use it when invoking Valgrind.
>This prepares for addition of Valgrind suppressions for current false
>positives and issues that cannot be fixed, preparing for enforcing
>Valgrind check.
>
>Make Valgrind output a suppression for every error and make it output
>used suppression names and counts at the end of each run. This
>simplifies discovery and addition of new suppressions and removal of
>unused ones.
>
>Related to https://fedorahosted.org/sssd/ticket/2428
>---
> Makefile.am          | 4 +++-
> contrib/ci/run       | 8 ++++++--
> contrib/ci/sssd.supp | 9 +++++++++
> 3 files changed, 18 insertions(+), 3 deletions(-)
> create mode 100644 contrib/ci/sssd.supp
>
>diff --git a/Makefile.am b/Makefile.am
>index b19d698..86a0572 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -342,7 +342,9 @@ dist_noinst_DATA = \
>     contrib/ci/configure.sh \
>     contrib/ci/deps.sh \
>     contrib/ci/distro.sh \
>-    contrib/ci/misc.sh
>+    contrib/ci/misc.sh \
>+    contrib/ci/sssd.supp \
>+    $(NULL)
> 
> ###############################
> # Global compilation settings #
>diff --git a/contrib/ci/run b/contrib/ci/run
>index 9864818..4619eb3 100755
>--- a/contrib/ci/run
>+++ b/contrib/ci/run
>@@ -18,7 +18,8 @@
> # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> 
> set -o nounset -o pipefail -o errexit
>-export PATH=`dirname "\`readlink -f \"\$0\"\`"`:$PATH
>+declare -r CI_DIR=`dirname "\`readlink -f \"\$0\"\`"`
>+export PATH=$CI_DIR:$PATH
> export LC_ALL=C
> 
> . deps.sh
>@@ -206,7 +207,10 @@ function build_debug()
>                                     --vgdb=no \
>                                     --trace-children=yes \
>                                     --trace-children-skip='*/bin/*,*/sbin/*' \
>-                                    --leak-check=full ||
>+                                    --leak-check=full \
>+                                    --gen-suppressions=all \
>+                                    --suppressions="$CI_DIR/sssd.supp" \
>+                                    --verbose ||
>             status=$?
>     mv "$test_dir" ci-test-dir
> 
>diff --git a/contrib/ci/sssd.supp b/contrib/ci/sssd.supp
>new file mode 100644
>index 0000000..d2541e8
>--- /dev/null
>+++ b/contrib/ci/sssd.supp
>@@ -0,0 +1,9 @@
>+#
>+# Valgrind suppression patterns
>+#
>+# See an introduction to suppressions in Valgrind manual:
>+# http://valgrind.org/docs/manual/manual-core.html#manual-core.suppress
>+#
>+# Each suppression name here must start with "sssd-" to differentiate it from
>+# suppressions maintained elsewhere.
>+#
>-- 
>2.1.0
>

>From 00fc96150e6bbc39bcb3deba1e54dbc623ab93e9 Mon Sep 17 00:00:00 2001
>From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>Date: Thu, 4 Sep 2014 14:36:40 +0300
>Subject: [PATCH 3/4] CI: Suppress all detected Valgrind issues
>
>Add suppressions for all issues detected by Valgrind during CI runs.
>These seem to be false positives, or cannot be fixed.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2428
>---
> contrib/ci/sssd.supp | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 99 insertions(+)
>
>diff --git a/contrib/ci/sssd.supp b/contrib/ci/sssd.supp
>index d2541e8..ea1e531 100644
>--- a/contrib/ci/sssd.supp
>+++ b/contrib/ci/sssd.supp
>@@ -7,3 +7,102 @@
> # Each suppression name here must start with "sssd-" to differentiate it from
> # suppressions maintained elsewhere.
> #
>+
>+# talloc-involved leaks
>+{
>+   sssd-leak-talloc
>+   Memcheck:Leak
>+   ...
>+   fun:talloc_*
>+   ...
>+}
>+{
>+   sssd-leak-_talloc
>+   Memcheck:Leak
>+   ...
>+   fun:_talloc_*
>+   ...
>+}
>+
>+# nss3-involved leaks
>+{
>+   sssd-leak-nss3
>+   Memcheck:Leak
>+   ...
>+   obj:*/libnss3.so
>+   ...
>+}
>+
>+# nspr4-involved leaks
>+{
>+   sssd-leak-nspr4
>+   Memcheck:Leak
>+   ...
>+   obj:*/libnspr4.so
>+   ...
>+}
>+{
>+   sssd-leak-nspr4-arena-allocate
>+   Memcheck:Leak
>+   fun:malloc
>+   fun:PL_ArenaAllocate
>+   ...
>+}
>+
>+# dbus-involved leaks
>+{
>+   sssd-leak-dbus
>+   Memcheck:Leak
>+   ...
>+   obj:*/libdbus-1.so.*
>+   ...
>+}
>+
>+# False positive - pcre_free is called in sss_names_ctx_destructor
>+{
>+   sssd-leak-sss_names
>+   Memcheck:Leak
>+   fun:malloc
>+   fun:pcre_compile2
>+   fun:sss_names_init_from_args
>+   ...
>+}
>+
>+# 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.
>+{
>+   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?

Indentaion should help to understand the code. I'm sorry it didin't help me.
It is possible that problem is in myself.

LS



More information about the sssd-devel mailing list