openldap-bugs

openldap-bugs@openldap.org

1 participants
21001 discussions

Re: (ITS#8707) slapd: Add systemd service notification support
by hyc＠symas.com 12 Sep '17

12 Sep '17

Ryan Tandy wrote: > On Tue, Sep 12, 2017 at 08:24:15PM +0000, hyc(a)symas.com wrote: >> I don't have documentation for sd_notify() on my machine > > https://www.freedesktop.org/software/systemd/man/sd_notify.html > >> - 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)? > > 0 in both of these cases (assuming there is not an unrelated NOTIFY_SOCKET env > var). However, it also says: > >> In order to support both service managers that implement this scheme and >> those which do not, it is generally recommended to ignore the return value >> of this call." > > and the current patch doesn't follow that recommendation, since it does check > the return value. I don't feel strongly either way about that. OK. You realize how egregiously gross this kind of API design is? "Oh, yeah, we return a value, but properly written programs should ignore it." Everything about this makes me nauseous. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by ryan＠nardis.ca 12 Sep '17

12 Sep '17

On Tue, Sep 12, 2017 at 08:24:15PM +0000, hyc(a)symas.com wrote: >I don't have documentation for sd_notify() on my machine https://www.freedesktop.org/software/systemd/man/sd_notify.html >- 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)? 0 in both of these cases (assuming there is not an unrelated NOTIFY_SOCKET env var). However, it also says: >In order to support both service managers that implement this scheme >and those which do not, it is generally recommended to ignore the >return value of this call." and the current patch doesn't follow that recommendation, since it does check the return value. I don't feel strongly either way about that. >It strikes me that this code should only be invoked if slapd was actually >started by systemd. As I understand it, sd_notify() should be a no-op (no error) if NOTIFY_SOCKET is not set. I don't think we should check NOTIFY_SOCKET ourselves; it's to some extent an implementation detail.

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by hyc＠symas.com 12 Sep '17

12 Sep '17

michael(a)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/

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by michael＠stroeder.com 12 Sep '17

12 Sep '17

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. Ciao, Michael.

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by ryan＠nardis.ca 12 Sep '17

12 Sep '17

On Tue, Sep 12, 2017 at 07:04:48PM +0000, hyc(a)symas.com wrote: >My experience so far with Arch/Debian/Ubuntu/Centos mirrors this - >they're all different in FS layouts (/var/run vs /run, etc etc etc) and >the situation is no better than it was in SysV init In the case of the unit file proposed, the only system-specific path is the path to slapd itself, and the patch already handles substituting that at install time. (OT: /run as a directory and /var/run as a symlink to it for backwards compatibility is the cross-distro standard today as far as I'm aware, and has been for a few years now. RHEL/CentOS 5/6 are the outliers since they predate /run but are still supported.) >If no one has any other reasons to offer, I'm inclined to reject and >close this ITS. Even if you don't want to ship the unit file, please consider at least accepting the code change to enable the startup notification, which should be completely distro-independent, and no more system-specific than any of our other optional library dependencies.

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by hyc＠symas.com 12 Sep '17

12 Sep '17

michael(a)stroeder.com wrote: > ryan(a)nardis.ca wrote: >> An explicit design goal of systemd is that unit files should be >> uniform enough that it's reasonable for upstream projects to >> ship them, >=20 > Yes, systemd has many admirable goals. And not enough competent developers to achieve any of them. > On the other hands Linux distributions differ a lot especially > regarding file system layout, not to speak of their systemd > back-port patches. My experience so far with Arch/Debian/Ubuntu/Centos mirrors this - they'r= e all=20 different in FS layouts (/var/run vs /run, etc etc etc) and the situation= is=20 no better than it was in SysV init. I see no reason for us to change our=20 policy regarding system-specific dependencies. re: providing an example in OpenLDAP documentation - this is an only slig= htly=20 less-bad suggestion. The onus is on systemd to provide adequate documenta= tion=20 on how to write unit files, and adequate examples of working files. Our o= nly=20 responsibility is to document the parameters for correctly running the co= de;=20 it's up to distro packagers to encapsulate a correct invocation into what= ever=20 their launch mechanism is. If no one has any other reasons to offer, I'm inclined to reject and clos= e=20 this ITS. >=20 > Ciao, Michael. >=20 > P.S.: > Right at this moment I'm trying to figure out the appropriate > Requires and After lines in systemd unit file template in =C3=83=E2=80=A0= -DIR's > ansible role. And the ansible role has only support for three (and > a half) different Linux distributions [1]. >=20 > [1] https://www.ae-dir.com/install.html#prereq >=20 >=20 >=20 >=20 --=20 -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#8671) Declare ldap_init_fd() in ldap.h to help external consumers
by quanah＠symas.com 12 Sep '17

12 Sep '17

Hi Alexander, Your submission appears to be missing an IPR statement as noted at <https://www.openldap.org/devel/contributing.html#notice>. This is required for your submission to be evaluated. Please add an IPR at your earliest convenience. Thanks! --Quanah --On Wednesday, June 07, 2017 11:44 AM +0000 abokovoy(a)redhat.com wrote: > > --ce5n3c4hsgyizewc > Content-Type: text/plain; charset=us-ascii; format=flowed > Content-Disposition: inline > > Patch attached. > > -- > / Alexander Bokovoy > > --ce5n3c4hsgyizewc > Content-Type: text/plain; charset=us-ascii > Content-Disposition: attachment; > filename="abokovoy-ldap_init_fd-move-ldap_init_fd-definition-to-ldap.h.pa > tch" Content-Transfer-Encoding: quoted-printable > > =46rom 2c1f3afd8098b2a345a900effa1a2e5c3f9e3b08 Mon Sep 17 00:00:00 2001 > =46rom: Alexander Bokovoy <abokovoy(a)redhat.com> > Date: Mon, 5 Jun 2017 14:43:58 +0300 > Subject: [PATCH] ldap_init_fd: move ldap_init_fd() definition to ldap.h > > While OpenLDAP team position is that all code in OpenLDAP source > distribution can be used by users, most distributions do not ship > ldap_pvt.h as part of openldap-devel (sub)package. > > Move ldap_init_fd() to ldap.h to facilitate external users who have a > need to integrate OpenLDAP with their own event loop processing. > --- > doc/man/man3/ldap_open.3 | 5 +---- > include/ldap.h | 8 ++++++++ > include/ldap_pvt.h | 2 -- > 3 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/doc/man/man3/ldap_open.3 b/doc/man/man3/ldap_open.3 > index 1d5e2b5..4f980f2 100644 > --- a/doc/man/man3/ldap_open.3 > +++ b/doc/man/man3/ldap_open.3 > @@ -3,7 +3,7 @@ > .\" Copyright 1998-2017 The OpenLDAP Foundation All Rights Reserved. > .\" Copying restrictions apply. See COPYRIGHT/LICENSE. > .SH NAME > -ldap_init, ldap_initialize, ldap_open \- Initialize the LDAP library and > o= pen a connection to an LDAP server > +ldap_init, ldap_initialize, ldap_open, ldap_init_fd \- Initialize the > LDAP= library and open a connection to an LDAP server > .SH LIBRARY > OpenLDAP LDAP (libldap, \-lldap) > .SH SYNOPSIS > @@ -45,9 +45,6 @@ LDAPURLDesc **url; > void *params; > .LP > .ft B > -#include <ldap_pvt.h> > -.LP > -.ft B > int ldap_init_fd(fd, proto, uri, ldp) > .ft > ber_socket_t fd; > diff --git a/include/ldap.h b/include/ldap.h > index 588e906..dd0c118 100644 > --- a/include/ldap.h > +++ b/include/ldap.h > @@ -1548,6 +1548,14 @@ LDAP_F( LDAP * ) > ldap_dup LDAP_P(( > LDAP *old )); > =20 > +/* open */ > +LDAP_F (int) > +ldap_init_fd LDAP_P(( > + ber_socket_t fd, > + int proto, > + LDAP_CONST char *url, > + LDAP **ldp )); > + > /* > * in tls.c > */ > diff --git a/include/ldap_pvt.h b/include/ldap_pvt.h > index f1d93ac..49f674c 100644 > --- a/include/ldap_pvt.h > +++ b/include/ldap_pvt.h > @@ -344,8 +344,6 @@ ldap_get_message_ber LDAP_P(( > /* open */ > LDAP_F (int) ldap_open_internal_connection LDAP_P(( > struct ldap **ldp, ber_socket_t *fdp )); > -LDAP_F (int) ldap_init_fd LDAP_P(( > - ber_socket_t fd, int proto, LDAP_CONST char *url, struct ldap **ldp )); > =20 > /* sasl.c */ > LDAP_F (int) ldap_pvt_sasl_generic_install LDAP_P(( Sockbuf *sb, > --=20 > 2.9.3 > > > --ce5n3c4hsgyizewc-- > > > -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by michael＠stroeder.com 12 Sep '17

12 Sep '17

ryan(a)nardis.ca wrote: > An explicit design goal of systemd is that unit files should be > uniform enough that it's reasonable for upstream projects to > ship them, Yes, systemd has many admirable goals. On the other hands Linux distributions differ a lot especially regarding file system layout, not to speak of their systemd back-port patches. Ciao, Michael. P.S.: Right at this moment I'm trying to figure out the appropriate Requires and After lines in systemd unit file template in Æ-DIR's ansible role. And the ansible role has only support for three (and a half) different Linux distributions [1]. [1] https://www.ae-dir.com/install.html#prereq

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by quanah＠symas.com 12 Sep '17

12 Sep '17

--On Tuesday, September 12, 2017 11:30 AM -0700 Ryan Tandy <ryan(a)nardis.ca> wrote: > "Part of this is that we want that unit files are supplied with upstream, > and not just added by the packager -- how it has usually been done in the > SysV world." > > Of course the unit file should only be installed if the configure switch > for systemd is enabled, and I believe the latest patch does this. I'll leave it to hyc to weigh in. It's not been the policy of the project so far to include such items. --Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>

1 0

Re: (ITS#8707) slapd: Add systemd service notification support
by ryan＠nardis.ca 12 Sep '17

12 Sep '17

On Tue, Sep 12, 2017 at 05:17:11PM +0000, quanah(a)symas.com wrote: >b) The OpenLDAP project has never provided init scripts of their >equivalents. I'm not sure it would be correct to include the systemd unit >file as a part of the project. An explicit design goal of systemd is that unit files should be uniform enough that it's reasonable for upstream projects to ship them, in contrast to init scripts where the conventions and helper scripts differ a lot from distro to distro. (This obviously does not preclude distro customization of the shipped unit file.) http://0pointer.de/blog/projects/on-etc-sysinit.html: "Part of this is that we want that unit files are supplied with upstream, and not just added by the packager -- how it has usually been done in the SysV world." Of course the unit file should only be installed if the configure switch for systemd is enabled, and I believe the latest patch does this.

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs