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 <[email protected]>
>>> >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 <[email protected]>
>>> >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
LS
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
