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

Lukas Slebodnik lslebodn at redhat.com
Wed Sep 17 16:48:47 UTC 2014


On (17/09/14 18:48), Nikolai Kondrashov wrote:
>On 09/17/2014 06:11 PM, Lukas Slebodnik wrote:
>>On (17/09/14 17:06), Nikolai Kondrashov wrote:
>>>--- a/Makefile.am
>>>+++ b/Makefile.am
>>>@@ -342,7 +342,8 @@ 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
>>We agreed to put $(NULL) at the end of list in Makefile.am
>>The benefit is that with next change just one line is added instead of 2+ 1-
>
>Sure. Updated patches attached. Thanks, Lukas.
>
>>We don't want to do mass changes, but you touch the list, si it is the best
>>time to do it.
>
>Hmm, I noticed that we practice such approach. Is it because of the merges?
>
>It seems to me it's just prolonging the pain and delaying the improvements.
>I would personally cut it off (do the mass change) and forget about it, but I
>probably never did that many merges.
>
>Nick

>From bcc5552e2c7955483f728e5b0bce75bef3ff34af 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..d3ecd38 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)
 ^^^
 '\t' instead of spaces
Please follow our conventions.

> 
> ###############################
> # 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 87272b8909049b6eb09973ad6cb022de5ac1a362 Mon Sep 17 00:00:00 2001
>From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>Date: Thu, 4 Sep 2014 17:09:31 +0300
>Subject: [PATCH 2/4] CI: Don't run dlopen-tests under Valgrind
>
>Disable running dlopen-tests under Valgrind as their use of dlclose
>makes Valgrind drop symbols and produce meaningless backtraces, which
>cannot be matched with specific suppressions.
>---
> contrib/ci/run | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/contrib/ci/run b/contrib/ci/run
>index 4619eb3..68ec1aa 100755
>--- a/contrib/ci/run
>+++ b/contrib/ci/run
>@@ -203,7 +203,8 @@ function build_debug()
>         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.

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


----------------

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

It seems that you don't use our git commit template
@see .git-commit-template
@see git show 3d9edb4c510028def2df41aa7b0ce705b197e6fc

LS



More information about the sssd-devel mailing list