michael@stroeder.com wrote:
Howard Chu wrote:
If no one has any other reasons to offer, I'm inclined to reject and close this ITS.
Note that the systemd unit file was only a little detail in this ITS. The most important part is the C code change.
OK. Looking at the patch:
+dnl +dnl Check for systemd +dnl +WITH_SYSTEMD=no +ol_link_systemd=no +if test $ol_with_systemd != no ; then + AC_CHECK_HEADERS(systemd/sd-daemon.h) + + if test $ac_cv_header_systemd_sd_daemon_h = yes; then + AC_CHECK_LIB(systemd, sd_notify, + [ol_link_systemd="-lsystemd"]) + fi + + if test $ol_link_systemd = no ; then + if test $ol_with_systemd != auto ; then + AC_MSG_ERROR([Could not locate systemd]) + else + AC_MSG_WARN([Could not locate systemd]) + AC_MSG_WARN([systemd service notification not supported!]) + fi + else + AC_DEFINE(HAVE_SYSTEMD,1,[define if you have systemd]) + SYSTEMD_LIBS="$ol_link_systemd" + WITH_SYSTEMD=yes + fi +fi +
There should not be any warning if with-systemd is defaulted to auto and it's not found. It should be silently ignored. This isn't like SASL, which is an explicit part of the LDAP spec and therefore has a warning in its absence.
+#ifdef HAVE_SYSTEMD + rc = sd_notifyf( 1, + "READY=1\n" + "STATUS=slapd: ready to serve connections...\n" + "MAINPID=%lu", + (unsigned long) getpid() ); + if ( rc < 0 ) + Debug( LDAP_DEBUG_ANY, + "systemd sd_notify failed (%d)\n", rc, 0, 0 ); +#endif /* HAVE_SYSTEMD */ +
I don't have documentation for sd_notify() on my machine - what does it return if systemd isn't running at the moment? What does it return if the current program wasn't started by systemd (and thus, the notification is irrelevant)? It strikes me that this code should only be invoked if slapd was actually started by systemd. In particular, I don't want to see a lot of "sd_notify failed" messages in our logs, for manually started slapd processes.