On (09/06/16 18:15), Jakub Hrozek wrote:
>On Thu, Jun 09, 2016 at 11:33:40AM +0200, Lukas Slebodnik wrote:
>> On (08/06/16 19:11), Lukas Slebodnik wrote:
>> >On (07/06/16 23:27), Jakub Hrozek wrote:
>> >>On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
>> >>> >diff --git a/src/external/systemtap.m4
b/src/external/systemtap.m4
>> >>> >new file mode 100644
>> >>> >index
0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba
>> >>> >--- /dev/null
>> >>> >+++ b/src/external/systemtap.m4
>> >>> >@@ -0,0 +1,35 @@
>> >>> >+dnl A macro to check the availability of systemtap user-space
probes
>> >>> >+AC_DEFUN([AM_CHECK_SYSTEMTAP],
>> >>> >+[
>> >>> >+ AC_ARG_ENABLE([systemtap],
>> >>> >+ [AS_HELP_STRING([--enable-systemtap],
>> >>> >+ [Enable inclusion of systemtap
trace support])],
>> >>> >+ [ENABLE_SYSTEMTAP="${enableval}"],
[ENABLE_SYSTEMTAP='no'])
>> >>> >+
>> >>> >+ if test "x${ENABLE_SYSTEMTAP}" = xyes; then
>> >>> >+ AC_CHECK_PROGS(DTRACE, dtrace)
>> >>> >+ if test -z "$DTRACE"; then
>> >>> >+ AC_MSG_ERROR([dtrace not found])
>> >>> >+ fi
>> >>> >+
>> >>> >+ AC_CHECK_HEADER([sys/sdt.h],
[SDT_H_FOUND='yes'],
>> >>> ^^^^^^^^^^^
>> >>> this variable is not used
anywhere.
>> >>> it would be better to use
default action
>> >>> "[]"
>> >>
>> >>Fixed
>> >>
>> >>> >+ [SDT_H_FOUND='no';
>> >>> >+ AC_MSG_ERROR([systemtap support needs
sys/sdt.h header])])
>> >>> e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK
headers]))
>> >>> >+
>> >>> >+ AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if
systemtap is enabled])
>> >>> >+ HAVE_SYSTEMTAP=1
>> >>> ^^^^^^^^^^^^^^
>> >>> it should already be set to 1 by AC_DEFINE.
>> >>
>> >>Didn't seem so, I thought AC_DEFINE really only sets the config.h
>> >>variable. When I removed this line, the AC_CONDITIONAL wasn't
evaluated.
>> >>
>> >You are right. I didn't remember it properly.
>> >AC_SUBST sets variable and not AC_DEFINE
>> >But we do not need to use substitution in makefile.am
>>
>http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Setting-Output-Variables.html#Setting-Output-Variables
>> >
>> >>> >+
>> >>> >+ AC_ARG_WITH([tapset-install-dir],
>> >>> >+
[AS_HELP_STRING([--with-tapset-install-dir],
>> >>> ^
>> >>> It would be good to append here string
"=DIR"
>> >>> So it will be clear that there is
expected argument
>> >>> We already have it for
"--with-systemdunitdir"
>> >>
>> >>Fixed
>> >>
>> >>> >+ [The absolute path where
the tapset dir will be installed])],
>> >>> >+ [if test "x${withval}" = x; then
>> >>> >+
tapset_dir="\$(datadir)/systemtap/tapset"
>> >>> >+ else
>> >>> >+ tapset_dir="${withval}"
>> >>> >+ fi],
>> >>> It can be simplified to
'tapset_dir="${withval}"' after
>> >>> updating help string.
>> >>
>> >>Fixed
>> >>
>> >>>
>> >>> >+
[tapset_dir="\$(datadir)/systemtap/tapset"])
>> >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >>> this default value could be
mentioned in
>> >>> help string as well.
>> >>> @see output of ./configure
--help
>> >>> (with-pubconf-path,
with-pipe-path ...)
>> >>
>> >>Fixed
>> >>
>> >>>
>> >>> >From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17
00:00:00 2001
>> >>> >From: Jakub Hrozek <jhrozek(a)redhat.com>
>> >>> >Date: Mon, 29 Feb 2016 13:20:28 +0100
>> >>> >Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track
sysdb transactions
>> >>> >
>> >>> >Actually adds marks for sysdb transactions that receive the
transaction
>> >>> >nesting level as an argument. The nesting is passed on from
probes to
>> >>> >marks along with a human-friendly description.
>> >>> >
>> >>> >The transaction commit is decorated with two probes, before and
after.
>> >>> >This would allow the caller to distinguish between the time we
spend in
>> >>> >the transaction (which might be important, because if a
transaction is
>> >>> >active on an ldb context, even the readers are blocked before
the
>> >>> >transaction completes) and the time we spend commiting the
transaction
>> >>> >(which is important because that's when the disk writes
occur)
>> >>> >
>> >>> >The probes would be installed into /usr/share/systemtap/tapset
on RHEL
>> >>> >and Fedora. This is in line with systemtap's paths which are
described
>> >>> >in detail in "man 7 stappaths".
>> >>> >---
>> >>> > Makefile.am | 4 +++-
>> >>> > configure.ac | 1 +
>> >>> > src/db/sysdb.c | 8 ++++++++
>> >>> > src/systemtap/sssd.stp.in | 32
++++++++++++++++++++++++++++++++
>> >>> > src/systemtap/sssd_probes.d | 4 ++++
>> >>> > 5 files changed, 48 insertions(+), 1 deletion(-)
>> >>> > create mode 100644 src/systemtap/sssd.stp.in
>> >>> >
>> >>> >diff --git a/Makefile.am b/Makefile.am
>> >>> >index
33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644
>> >>> >--- a/Makefile.am
>> >>> >+++ b/Makefile.am
>> >>> >@@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@
>> >>> > sudolibdir = @sudolibpath@
>> >>> > polkitdir = @polkitdir@
>> >>> > pamconfdir = $(sysconfdir)/pam.d
>> >>> >+systemtap_tapdir = @tapset_dir@
>> >>> >
>> >>> > UNICODE_LIBS=@UNICODE_LIBS@
>> >>> >
>> >>> >@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \
>> >>> > $(srcdir)/src/systemtap/sssd_probes.d \
>> >>> > $(NULL)
>> >>> >
>> >>> >+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp
>> >>> >+
>> >>> Do we need to install this file if sssd is not build with systemtap
support?
>> >>
>> >>I think we need to distribute it in the tarball, but not install without
>> >>systemtap. So I guess we can use dist_noist_DATA if systemtap is not
>> >>configured?
>> >>
>> >It will be part of tarball even if is part of conditional build.
>> >if BUILD_SYSTEMTAP
>> >...
>> >
>> >
>> >
>> >>>
>> >>> The same applies to other patches
>> >>> and automake variables dist_systemtap_tap_DATA,
dist_sssdtapscript_DATA
>> >>>
>> >>> It caused mock failures
>> >>> RPM build errors:
>> >>> error: Installed (but unpackaged) file(s) found:
>> >>> /usr/share/sssd/systemtap/id_perf.stp
>> >>> Empty %files file
/builddir/build/BUILD/sssd-1.13.90/sssd_client.lang
>> >>> Empty %files file
/builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang
>> >>> Empty %files file
/builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang
>> >>> Empty %files file
/builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang
>> >>> Empty %files file
/builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang
>> >>> Empty %files file
/builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang
>> >>> Installed (but unpackaged) file(s) found:
>> >>> /usr/share/sssd/systemtap/id_perf.stp
>> >>
>> >>Sorry, can't reproduce it here. Did you run make srpms from
Makefile?
>> >>
>> >I forgot to mention that you need to test without spec file chages.
>> >But following diff fixed issue.
>> >
>> >diff --git a/Makefile.am b/Makefile.am
>> >index 3a9858f..7bcac65 100644
>> >--- a/Makefile.am
>> >+++ b/Makefile.am
>> >@@ -1079,6 +1079,7 @@ endif
>> > # Systemtap tracing #
>> > #########################
>> >
>> >+if BUILD_SYSTEMTAP
>> > SYSTEMTAP_PROBES = \
>> > $(srcdir)/src/systemtap/sssd_probes.d \
>> > $(NULL)
>> >@@ -1088,7 +1089,11 @@ dist_systemtap_tap_DATA = \
>> > $(builddir)/src/systemtap/sssd_functions.stp \
>> > $(NULL)
>> >
>> >-if BUILD_SYSTEMTAP
>> >+dist_sssdtapscript_DATA = \
>> >+ contrib/systemtap/id_perf.stp \
>> >+ contrib/systemtap/nested_group_perf.stp \
>> >+ $(NULL)
>> >+
>> > stap_generated_probes.h: $(srcdir)/src/systemtap/sssd_probes.d
>> > $(AM_V_GEN)$(DTRACE) -C -h -s $< -o $@
>> >
>> >@@ -1112,11 +1117,6 @@ CLEANFILES += stap_generated_probes.h \
>> > $(NULL)
>> > endif
>> >
>> >-dist_sssdtapscript_DATA = \
>> >- contrib/systemtap/id_perf.stp \
>> >- contrib/systemtap/nested_group_perf.stp \
>> >- $(NULL)
>> >-
>> >
>> >I moved dist_sssdtapscript_DATA closer to the dist_systemtap_tap_DATA
>> >so it is part of conditional build and IMHO it's nicer if they are
together :-)
>> >
>> >>>
>> >>> Lang files might be unrelated but there are *.stp files in output.
>> >>>
http://sssd-ci.duckdns.org/logs/job/44/50/summary.html
>> >>> It works after applying the last patch from patchset.
>> >>>
>> >>>
>> >>> > int sysdb_transaction_commit(struct sysdb_ctx *sysdb)
>> >>> > {
>> >>> > int ret;
>> >>> >+#ifdef HAVE_SYSTEMTAP
>> >>> >+ int commit_nesting = sysdb->transaction_nesting-1;
>> >>> ^^
>> >>> missing spaces :-)
>> >>> >+#endif
>> >>>
>> >>> >From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17
00:00:00 2001
>> >>> >From: Jakub Hrozek <jhrozek(a)redhat.com>
>> >>> >Date: Fri, 6 May 2016 15:51:12 +0200
>> >>> >Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build
and CI
>> >>> >
>> >>> >So far, systemtap is only enabled for Red Hat distributions.
>> >>> >---
>> >>> > contrib/ci/configure.sh | 6 ++++++
>> >>> > contrib/sssd.spec.in | 19 +++++++++++++++++++
>> >>> > 2 files changed, 25 insertions(+)
>> >>> >
>> >>> >diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh
>> >>> >index
c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644
>> >>> >--- a/contrib/ci/configure.sh
>> >>> >+++ b/contrib/ci/configure.sh
>> >>> >@@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" ==
-redhat-redhatenterprise*-7.*- ||
>> >>> > )
>> >>> > fi
>> >>> >
>> >>> >+if [[ "$DISTRO_FAMILY" == "redhat" ]];
then
>> >>> >+ CONFIGURE_ARG_LIST+=(
>> >>> >+ "--enable-systemtap"
>> >>> >+ )
>> >>> >+fi
>> >>> >+
>> >>> Maybe we can enable a build also on debian with installed package
>> >>> systemtap-sdt-dev (I didn't try)
>> >>
>> >>Me neither so far, I just sent a CI build to see what happens..
>> >>
>> >I could not see any issue on debian with following change
>> >diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh
>> >index 823d327..8e779cf 100644
>> >--- a/contrib/ci/configure.sh
>> >+++ b/contrib/ci/configure.sh
>> >@@ -28,6 +28,7 @@ declare -a CONFIGURE_ARG_LIST=(
>> > "--disable-static"
>> > "--enable-ldb-version-check"
>> > "--with-syslog=journald"
>> >+ "--enable-systemtap"
>> > )
>> >
>> >
>> >@@ -47,12 +48,6 @@ if [[ "$DISTRO_BRANCH" ==
-redhat-redhatenterprise*-7.*- ||
>> > )
>> > fi
>> >
>> >-if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
>> >- CONFIGURE_ARG_LIST+=(
>> >- "--enable-systemtap"
>> >- )
>> >-fi
>> >-
>> > declare -r -a CONFIGURE_ARG_LIST
>> >
>> > fi # _CONFIGURE_SH
>> >diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh
>> >index c9a8a63..4fc1d2d 100644
>> >--- a/contrib/ci/deps.sh
>> >+++ b/contrib/ci/deps.sh
>> >@@ -114,6 +114,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then
>> > python-ldap
>> > ldap-utils
>> > slapd
>> >+ systemtap-sdt-dev
>> > )
>> > DEPS_INTGCHECK_SATISFIED=true
>> > fi
>> >
>> >
>> >>>
>> >>> I briefly look at system tap script and they looks good to me.
>> >>> some lines are longer than 80 characters :-)
>> >>> and there could be more comments but it's not a blocker.
>> >>
>> >>OK, so far I added some comments and pushed along with the first fixes
>> >>to github. I will wait for the next round of patches until we figure out
>> >>the distcheck failures.
>> >which distcheck failures do you mean?
>> >
>> BTW I also noticed that tarball contains src/systemtap/sssd.stp and
>> also template src/systemtap/sssd.stp.in
>>
>> IMHO there should be only template
>>
>> tar -tnvzf sssd-1.13.90.tar.gz | grep systemtap
>> drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/src/systemtap/
>> -rw-rw-r-- user/user 7988 2016-06-09 11:29
sssd-1.13.90/src/systemtap/sssd.stp.in
>> -rw-rw-r-- user/user 2201 2016-06-09 11:29
sssd-1.13.90/src/systemtap/sssd_functions.stp
>> -rw-rw-r-- user/user 8408 2016-06-09 11:30
sssd-1.13.90/src/systemtap/sssd.stp
>> -rw-rw-r-- user/user 2682 2016-06-09 11:29
sssd-1.13.90/src/systemtap/sssd_probes.d
>> -rw-rw-r-- user/user 1385 2016-06-09 11:29
sssd-1.13.90/src/external/systemtap.m4
>> drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/contrib/systemtap/
>> -rw-rw-r-- user/user 9614 2016-06-09 11:29
sssd-1.13.90/contrib/systemtap/nested_group_perf.stp
>> -rw-rw-r-- user/user 4991 2016-06-09 11:29
sssd-1.13.90/contrib/systemtap/id_perf.stp
>
>Thank you for the review, the attached patches should hopefully fix all
>the issues.
http://sssd-ci.duckdns.org/logs/job/45/06/summary.html
ACK
master:
* acf7cee13f07b368b0ccae69776309f7f69cbca1
* 94b860e8fc51c659c6ac09d69481b838555fff76
* c23ea7772113a163139a7b7669303e9e80dc1d09
* 41291f19dbc5bf14f20729959b852fa605fcc02d
* 630f3ff08c1d17c7900b9bde814922f775ca2703
* 8c829226ce0cf98c35ffce39a66f9645cff65767
* 6dcbfe52d5e64205c0d922f3e89add066b42c496
* bd93ef2db6d24946ebf98a23fa18d34d45f6b072
* 29c5542feb4c45865ea61be97e0e84a1d1f04918
* 53f1b03f4e61ebe21df0c2fd05e09e0504fd8881
LS