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