Re: (ITS#8707) slapd: Add systemd service notification support

[hyc@symas.com]

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.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/