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/h...
> >+
> >+ 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?
LS