diff mbox

[1/1] openldap: add support to build the server

Message ID 1450384879-5494-1-git-send-email-universeII@gmx.de
State Changes Requested
Headers show

Commit Message

universe II Dec. 17, 2015, 8:41 p.m. UTC
Signed-off-by: Andreas Ehmanns <universeII@gmx.de>
---
 package/openldap/Config.in   |  8 ++++++-
 package/openldap/S75slapd    | 51 ++++++++++++++++++++++++++++++++++++++++++++
 package/openldap/openldap.mk | 17 ++++++++++++++-
 3 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100644 package/openldap/S75slapd

Comments

Thomas Petazzoni Dec. 29, 2015, 11:19 a.m. UTC | #1
Andreas,

Thanks for this patch. I tried it, but there are a number of remaining
issues to be resolved I believe.

First, if you disable BR2_PACKAGE_OPENLDAP_CLIENTS,
the /etc/openldap/slapd.conf file gets removed in a post installation
hook, so slapd cannot start.

If you fix this, then the path to the pidfile (and argsfile) in
slapd.conf are wrong, because they point to /var/run/, to which the
ldap user has not write access.

If you fix this again, when you start slapd, it complains:

bdb_db_open: warning - no DB_CONFIG file found in
directory /var/openldap-data: (2). Expect poor performance for suffix
"dc=my-domain,dc=com".

It should probably be fixed by using DB_CONFIG.example as DB_CONFIG
in /var/openldap-data/.

Some more comments below.

On Thu, 17 Dec 2015 21:41:19 +0100, Andreas Ehmanns wrote:

> +case "$1" in
> +    start)
> +        if [ ! -d /var/run/openldap ]; then
> +            install -d -o ldap -g ldap -m 755 /var/run/openldap
> +        fi
> +
> +        if [ ! -d /var/openldap-data ]; then
> +            install -d -o ldap -g ldap -m 755 /var/openldap-data

This directory should be 700 according to the slapd documentation:

==
# The database directory MUST exist prior to running slapd AND 
# should only be accessible by the slapd and slap tools.
# Mode 700 recommended.
directory       %LOCALSTATEDIR%/openldap-data
==

> +        else
> +            chown -R ldap:ldap /var/openldap-data
> +        fi

It is not clear why you need this. /var is a persistent directory, so I
believe all you need here is an unconditional:

	chown -R ldap:ldap /var/openldap-data

Setting the permission to 700 can be done by a OPENLDAP_PERMISSIONS
variable in the .mk file. Ideally, we would also be able to define the
user/group, but we currently can't do this by referencing symbolic
user/groups, only by explicit UID/GID, and we don't know the UID/GID
that will be allocated to the ldap user/group. So I think we should:

 1/ Set the permission in OPENLDAP_PERMISSIONS
 2/ Set the owner/group in the S75slapd script

> +
> +        printf "Starting $DESC: $NAME: "
> +        start-stop-daemon -S -b -n $NAME -a $DAEMON -- $ARGS

You can add:

	-p /var/run/slapd/slapd.pid

Why do you pass -n ? And why do you use -a instead of -x ?

See S50dropbear in the Buildroot sources for a good example of an init
script.

> +        echo "done."
> +        ;;
> +    stop)
> +        printf "Stopping $DESC: $NAME: "
> +        start-stop-daemon -K -n $NAME

Same here.

Also add the "-q" option

> +        echo "done."
> +        ;;
> +    restart)
> +        printf "Restarting $DESC: $NAME: "
> +        $0 stop
> +        $0 start
> +        echo "done."
> +        ;;
> +    reload)
> +        printf "Reloading $DESC: $NAME: "
> +        killall -HUP $(basename ${DAEMON})

I think it's better to use the pid file here, no?

	   kill -HUP $(cat /var/run/slapd/slapd.pid)

> +        echo "done."
> +        ;;
> +    *)
> +        echo "Usage: $0 {start|stop|restart|reload}"
> +        exit 1
> +        ;;
> +esac
> +
> +exit 0
> +
> +
> diff --git a/package/openldap/openldap.mk b/package/openldap/openldap.mk
> index 17bf991..bcb285a 100644
> --- a/package/openldap/openldap.mk
> +++ b/package/openldap/openldap.mk
> @@ -12,6 +12,17 @@ OPENLDAP_LICENSE_FILES = LICENSE
>  OPENLDAP_INSTALL_STAGING = YES
>  OPENLDAP_DEPENDENCIES = host-pkgconf
>  
> +ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),y)
> +define OPENLDAP_USERS
> +        ldap -1 ldap -1 * /var/run/openldap - - OpenLDAP server user
> +endef
> +
> +define OPENLDAP_COPY_INITSCRIPT
> +        $(INSTALL) -D -m 755 $(@D)/S75slapd $(TARGET_DIR)/etc/init.d/S75slapd

$(@D)/S75slapd does not exist. So this means you never rebuilt your
package :-)

$(@D) is the source directory of openldap. You want to replace this
with: $(OPENLDAP_PKGDIR)/S75slapd

> +endef
> +OPENLDAP_POST_INSTALL_TARGET_HOOKS += OPENLDAP_COPY_INITSCRIPT

Shouldn't be a post install target hook. Instead, do this:

define OPENLDAP_INIT_SYSV
	$(INSTALL) -D -m 755 $(OPENLDAP_PKGDIR)/S75slapd $(TARGET_DIR)/etc/init.d/S75slapd
endef

and it will automatically install the init script of the chosen init
system is sysV compatible.

> +endif
> +
>  ifeq ($(BR2_PACKAGE_OPENSSL),y)
>  OPENLDAP_TLS = openssl
>  OPENLDAP_DEPENDENCIES += openssl
> @@ -44,7 +55,6 @@ OPENLDAP_CONF_ENV += ac_cv_func_memcmp_working=yes
>  OPENLDAP_CONF_OPTS += \
>  	--enable-syslog \
>  	--disable-proctitle \
> -	--disable-slapd \
>  	--with-yielding-select \
>  	--sysconfdir=/etc \
>  	--enable-dynamic=$(if $(BR2_STATIC_LIBS),no,yes) \
> @@ -52,6 +62,11 @@ OPENLDAP_CONF_OPTS += \
>  	--with-mp=$(OPENLDAP_MP) \
>  	CPPFLAGS="$(TARGET_CPPFLAGS) $(OPENLDAP_CPPFLAGS)"
>  
> +ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),)
> +OPENLDAP_CONF_OPTS += \
> +        --disable-slapd
> +endif

Please do:

ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),y)
OPENLDAP_CONF_OPTS += --enable-slapd
else
OPENLDAP_CONF_OPTS += --disable-slapd
endif

Which is a bit more explicit.

Could you rework your patch to solve those different issues, and send
an updated version?

Thanks!

Thomas
universe II Jan. 3, 2016, 2:07 p.m. UTC | #2
Thomas,
thanks for your detailed answer. Actually I'm on holiday but will be 
back on 11th of January.
Then I will have a look at all your comments, fix the issues and prepare 
a new patch.

Regards,
Andreas


Am 29.12.2015 um 12:19 schrieb Thomas Petazzoni:
> Andreas,
>
> Thanks for this patch. I tried it, but there are a number of remaining
> issues to be resolved I believe.
>
> First, if you disable BR2_PACKAGE_OPENLDAP_CLIENTS,
> the /etc/openldap/slapd.conf file gets removed in a post installation
> hook, so slapd cannot start.
>
> If you fix this, then the path to the pidfile (and argsfile) in
> slapd.conf are wrong, because they point to /var/run/, to which the
> ldap user has not write access.
>
> If you fix this again, when you start slapd, it complains:
>
> bdb_db_open: warning - no DB_CONFIG file found in
> directory /var/openldap-data: (2). Expect poor performance for suffix
> "dc=my-domain,dc=com".
>
> It should probably be fixed by using DB_CONFIG.example as DB_CONFIG
> in /var/openldap-data/.
>
> Some more comments below.
>
> On Thu, 17 Dec 2015 21:41:19 +0100, Andreas Ehmanns wrote:
>
>> +case "$1" in
>> +    start)
>> +        if [ ! -d /var/run/openldap ]; then
>> +            install -d -o ldap -g ldap -m 755 /var/run/openldap
>> +        fi
>> +
>> +        if [ ! -d /var/openldap-data ]; then
>> +            install -d -o ldap -g ldap -m 755 /var/openldap-data
> This directory should be 700 according to the slapd documentation:
>
> ==
> # The database directory MUST exist prior to running slapd AND
> # should only be accessible by the slapd and slap tools.
> # Mode 700 recommended.
> directory       %LOCALSTATEDIR%/openldap-data
> ==
>
>> +        else
>> +            chown -R ldap:ldap /var/openldap-data
>> +        fi
> It is not clear why you need this. /var is a persistent directory, so I
> believe all you need here is an unconditional:
>
> 	chown -R ldap:ldap /var/openldap-data
>
> Setting the permission to 700 can be done by a OPENLDAP_PERMISSIONS
> variable in the .mk file. Ideally, we would also be able to define the
> user/group, but we currently can't do this by referencing symbolic
> user/groups, only by explicit UID/GID, and we don't know the UID/GID
> that will be allocated to the ldap user/group. So I think we should:
>
>   1/ Set the permission in OPENLDAP_PERMISSIONS
>   2/ Set the owner/group in the S75slapd script
>
>> +
>> +        printf "Starting $DESC: $NAME: "
>> +        start-stop-daemon -S -b -n $NAME -a $DAEMON -- $ARGS
> You can add:
>
> 	-p /var/run/slapd/slapd.pid
>
> Why do you pass -n ? And why do you use -a instead of -x ?
>
> See S50dropbear in the Buildroot sources for a good example of an init
> script.
>
>> +        echo "done."
>> +        ;;
>> +    stop)
>> +        printf "Stopping $DESC: $NAME: "
>> +        start-stop-daemon -K -n $NAME
> Same here.
>
> Also add the "-q" option
>
>> +        echo "done."
>> +        ;;
>> +    restart)
>> +        printf "Restarting $DESC: $NAME: "
>> +        $0 stop
>> +        $0 start
>> +        echo "done."
>> +        ;;
>> +    reload)
>> +        printf "Reloading $DESC: $NAME: "
>> +        killall -HUP $(basename ${DAEMON})
> I think it's better to use the pid file here, no?
>
> 	   kill -HUP $(cat /var/run/slapd/slapd.pid)
>
>> +        echo "done."
>> +        ;;
>> +    *)
>> +        echo "Usage: $0 {start|stop|restart|reload}"
>> +        exit 1
>> +        ;;
>> +esac
>> +
>> +exit 0
>> +
>> +
>> diff --git a/package/openldap/openldap.mk b/package/openldap/openldap.mk
>> index 17bf991..bcb285a 100644
>> --- a/package/openldap/openldap.mk
>> +++ b/package/openldap/openldap.mk
>> @@ -12,6 +12,17 @@ OPENLDAP_LICENSE_FILES = LICENSE
>>   OPENLDAP_INSTALL_STAGING = YES
>>   OPENLDAP_DEPENDENCIES = host-pkgconf
>>   
>> +ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),y)
>> +define OPENLDAP_USERS
>> +        ldap -1 ldap -1 * /var/run/openldap - - OpenLDAP server user
>> +endef
>> +
>> +define OPENLDAP_COPY_INITSCRIPT
>> +        $(INSTALL) -D -m 755 $(@D)/S75slapd $(TARGET_DIR)/etc/init.d/S75slapd
> $(@D)/S75slapd does not exist. So this means you never rebuilt your
> package :-)
>
> $(@D) is the source directory of openldap. You want to replace this
> with: $(OPENLDAP_PKGDIR)/S75slapd
>
>> +endef
>> +OPENLDAP_POST_INSTALL_TARGET_HOOKS += OPENLDAP_COPY_INITSCRIPT
> Shouldn't be a post install target hook. Instead, do this:
>
> define OPENLDAP_INIT_SYSV
> 	$(INSTALL) -D -m 755 $(OPENLDAP_PKGDIR)/S75slapd $(TARGET_DIR)/etc/init.d/S75slapd
> endef
>
> and it will automatically install the init script of the chosen init
> system is sysV compatible.
>
>> +endif
>> +
>>   ifeq ($(BR2_PACKAGE_OPENSSL),y)
>>   OPENLDAP_TLS = openssl
>>   OPENLDAP_DEPENDENCIES += openssl
>> @@ -44,7 +55,6 @@ OPENLDAP_CONF_ENV += ac_cv_func_memcmp_working=yes
>>   OPENLDAP_CONF_OPTS += \
>>   	--enable-syslog \
>>   	--disable-proctitle \
>> -	--disable-slapd \
>>   	--with-yielding-select \
>>   	--sysconfdir=/etc \
>>   	--enable-dynamic=$(if $(BR2_STATIC_LIBS),no,yes) \
>> @@ -52,6 +62,11 @@ OPENLDAP_CONF_OPTS += \
>>   	--with-mp=$(OPENLDAP_MP) \
>>   	CPPFLAGS="$(TARGET_CPPFLAGS) $(OPENLDAP_CPPFLAGS)"
>>   
>> +ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),)
>> +OPENLDAP_CONF_OPTS += \
>> +        --disable-slapd
>> +endif
> Please do:
>
> ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),y)
> OPENLDAP_CONF_OPTS += --enable-slapd
> else
> OPENLDAP_CONF_OPTS += --disable-slapd
> endif
>
> Which is a bit more explicit.
>
> Could you rework your patch to solve those different issues, and send
> an updated version?
>
> Thanks!
>
> Thomas
universe II Jan. 12, 2016, 9:02 p.m. UTC | #3
Thomas,
I reworked the patch and incorporated your findings. Please have a look 
at my comments below and let me know what you think.


Am 29.12.2015 um 12:19 schrieb Thomas Petazzoni:
> Andreas,
>
> Thanks for this patch. I tried it, but there are a number of remaining
> issues to be resolved I believe.
>
> First, if you disable BR2_PACKAGE_OPENLDAP_CLIENTS,
> the /etc/openldap/slapd.conf file gets removed in a post installation
> hook, so slapd cannot start.
>
> If you fix this, then the path to the pidfile (and argsfile) in
> slapd.conf are wrong, because they point to /var/run/, to which the
> ldap user has not write access.
>
> If you fix this again, when you start slapd, it complains:
>
> bdb_db_open: warning - no DB_CONFIG file found in
> directory /var/openldap-data: (2). Expect poor performance for suffix
> "dc=my-domain,dc=com".
>
> It should probably be fixed by using DB_CONFIG.example as DB_CONFIG
> in /var/openldap-data/.
My aim was to add the OpenLDAP server as provided by the package and 
only make the changes necessary to allow the server to start up without 
terminating.
slapd.conf is the default configuration provided by the package which is 
a good starting point for people to setup their own configuration and 
database. Of course everyone using the LDAP server has to make its own 
configuration and database setup but this can't be provided or 
preconfigured by buildroot.
> Some more comments below.
>
> On Thu, 17 Dec 2015 21:41:19 +0100, Andreas Ehmanns wrote:
>
>> +case "$1" in
>> +    start)
>> +        if [ ! -d /var/run/openldap ]; then
>> +            install -d -o ldap -g ldap -m 755 /var/run/openldappackage
>> +        fi
>> +
>> +        if [ ! -d /var/openldap-data ]; then
>> +            install -d -o ldap -g ldap -m 755 /var/openldap-data
> This directory should be 700 according to the slapd documentation:
O.k., changed it.
>
> ==
> # The database directory MUST exist prior to running slapd AND
> # should only be accessible by the slapd and slap tools.
> # Mode 700 recommended.
> directory       %LOCALSTATEDIR%/openldap-data
> ==
>
>> +        else
>> +            chown -R ldap:ldap /var/openldap-data
>> +        fi
> It is not clear why you need this. /var is a persistent directory, so I
> believe all you need here is an unconditional:
o.k., changed it.
>
> 	chown -R ldap:ldap /var/openldap-data
>
> Setting the permission to 700 can be done by a OPENLDAP_PERMISSIONS
> variable in the .mk file. Ideally, we would also be able to define the
> user/group, but we currently can't do this by referencing symbolic
> user/groups, only by explicit UID/GID, and we don't know the UID/GID
> that will be allocated to the ldap user/group. So I think we should:
>
>   1/ Set the permission in OPENLDAP_PERMISSIONS
>   2/ Set the owner/group in the S75slapd script
>
>> +
>> +        printf "Starting $DESC: $NAME: "
>> +        start-stop-daemon -S -b -n $NAME -a $DAEMON -- $ARGS
> You can add:
>
> 	-p /var/run/slapd/slapd.pid
Slapd manages its own pid file. Why should start-stop-daemon create an 
additional pid file
>
> Why do you pass -n ? And why do you use -a instead of -x ?
O.k., changed -a to -x
I thought that I need -n to be able to do a kill when shutting down the 
server when NOT using pid file from start-stop-daemon. This was my 
understanding from other init scripts. Am I wrong?
>
> See S50dropbear in the Buildroot sources for a good example of an init
> script.
>
>> +        echo "done."
>> +        ;;
>> +    stop)
>> +        printf "Stopping $DESC: $NAME: "
>> +        start-stop-daemon -K -n $NAME
> Same here.
>
> Also add the "-q" option
O.k., added -q option
>
>> +        echo "done."
>> +        ;;
>> +    restart)
>> +        printf "Restarting $DESC: $NAME: "
>> +        $0 stop
>> +        $0 start
>> +        echo "done."
>> +        ;;
>> +    reload)
>> +        printf "Reloading $DESC: $NAME: "
>> +        killall -HUP $(basename ${DAEMON})
> I think it's better to use the pid file here, no?
>
> 	   kill -HUP $(cat /var/run/slapd/slapd.pid)
See comment above. Slapd is managing its own pid file.
>
>> +        echo "done."
>> +        ;;
>> +    *)
>> +        echo "Usage: $0 {start|stop|restart|reload}"
>> +        exit 1
>> +        ;;
>> +esac
>> +
>> +exit 0
>> +
>> +
>> diff --git a/package/openldap/openldap.mk b/package/openldap/openldap.mk
>> index 17bf991..bcb285a 100644
>> --- a/package/openldap/openldap.mk
>> +++ b/package/openldap/openldap.mk
>> @@ -12,6 +12,17 @@ OPENLDAP_LICENSE_FILES = LICENSE
>>   OPENLDAP_INSTALL_STAGING = YES
>>   OPENLDAP_DEPENDENCIES = host-pkgconf
>>   
>> +ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),y)
>> +define OPENLDAP_USERS
>> +        ldap -1 ldap -1 * /var/run/openldap - - OpenLDAP server user
>> +endef
>> +
>> +define OPENLDAP_COPY_INITSCRIPT
>> +        $(INSTALL) -D -m 755 $(@D)/S75slapd $(TARGET_DIR)/etc/init.d/S75slapd
> $(@D)/S75slapd does not exist. So this means you never rebuilt your
> package :-)
>
> $(@D) is the source directory of openldap. You want to replace this
> with: $(OPENLDAP_PKGDIR)/S75slapd
Fixed an re-tested.
>
>> +endef
>> +OPENLDAP_POST_INSTALL_TARGET_HOOKS += OPENLDAP_COPY_INITSCRIPT
> Shouldn't be a post install target hook. Instead, do this:
>
> define OPENLDAP_INIT_SYSV
> 	$(INSTALL) -D -m 755 $(OPENLDAP_PKGDIR)/S75slapd $(TARGET_DIR)/etc/init.d/S75slapd
> endef
O.k., changed it.
>
> and it will automatically install the init script of the chosen init
> system is sysV compatible.
>
>> +endif
>> +
>>   ifeq ($(BR2_PACKAGE_OPENSSL),y)
>>   OPENLDAP_TLS = openssl
>>   OPENLDAP_DEPENDENCIES += openssl
>> @@ -44,7 +55,6 @@ OPENLDAP_CONF_ENV += ac_cv_func_memcmp_working=yes
>>   OPENLDAP_CONF_OPTS += \
>>   	--enable-syslog \
>>   	--disable-proctitle \
>> -	--disable-slapd \
>>   	--with-yielding-select \
>>   	--sysconfdir=/etc \
>>   	--enable-dynamic=$(if $(BR2_STATIC_LIBS),no,yes) \
>> @@ -52,6 +62,11 @@ OPENLDAP_CONF_OPTS += \
>>   	--with-mp=$(OPENLDAP_MP) \
>>   	CPPFLAGS="$(TARGET_CPPFLAGS) $(OPENLDAP_CPPFLAGS)"
>>   
>> +ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),)
>> +OPENLDAP_CONF_OPTS += \
>> +        --disable-slapd
>> +endif
> Please do:
>
> ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),y)
> OPENLDAP_CONF_OPTS += --enable-slapd
> else
> OPENLDAP_CONF_OPTS += --disable-slapd
> endif
>
> Which is a bit more explicit.
Agree. Changed it.
>
> Could you rework your patch to solve those different issues, and send
> an updated version?
>
> Thanks!
>
> Thomas

Instead of a patch I attached you the two files under discussion 
(openldap.mk and S75slapd) to make review easier. If you agree with the 
changes I will send a full patch using git.

Regards,
Andreas
#!/bin/sh
DAEMON=/usr/libexec/slapd
NAME=slapd
DESC="OpenLDAP server"
CONF=/etc/openldap/slapd.conf

ARGS="-u ldap -g ldap"

set -e

case "$1" in
    start)
        if [ ! -d /var/run/openldap ]; then
            install -d -o ldap -g ldap -m 755 /var/run/openldap
        fi

        chown -R ldap:ldap /var/openldap-data

        printf "Starting $DESC: $NAME: "
        start-stop-daemon -S -q -n $NAME -x $DAEMON -- $ARGS
        echo "done."
        ;;
    stop)
        printf "Stopping $DESC: $NAME: "
        start-stop-daemon -K -n $NAME
        echo "done."
        ;;
    restart)
        printf "Restarting $DESC: $NAME: "
        $0 stop
        $0 start
        echo "done."
        ;;
    reload)
        printf "Reloading $DESC: $NAME: "
        killall -HUP $(basename ${DAEMON})
        echo "done."
        ;;
    *)
        echo "Usage: $0 {start|stop|restart|reload}"
        exit 1
        ;;
esac

exit 0
Thomas Petazzoni Jan. 12, 2016, 9:09 p.m. UTC | #4
Andreas,

On Tue, 12 Jan 2016 22:02:28 +0100, Andreas Ehmanns wrote:

> I reworked the patch and incorporated your findings. Please have a look 
> at my comments below and let me know what you think.

Thanks! See below my comments.


> > If you fix this, then the path to the pidfile (and argsfile) in
> > slapd.conf are wrong, because they point to /var/run/, to which the
> > ldap user has not write access.
> >
> > If you fix this again, when you start slapd, it complains:
> >
> > bdb_db_open: warning - no DB_CONFIG file found in
> > directory /var/openldap-data: (2). Expect poor performance for suffix
> > "dc=my-domain,dc=com".
> >
> > It should probably be fixed by using DB_CONFIG.example as DB_CONFIG
> > in /var/openldap-data/.
> My aim was to add the OpenLDAP server as provided by the package and 
> only make the changes necessary to allow the server to start up without 
> terminating.
> slapd.conf is the default configuration provided by the package which is 
> a good starting point for people to setup their own configuration and 
> database. Of course everyone using the LDAP server has to make its own 
> configuration and database setup but this can't be provided or 
> preconfigured by buildroot.

Right, but in general we try in Buildroot to provide a sane/minimal
default configuration that "works" out of the box. It is a bit weird to
have such a warning when the slapd daemon starts. But OK, it's not a
very big issue either, we can always leave it as it is for now for this
aspect.

> > 	-p /var/run/slapd/slapd.pid
> Slapd manages its own pid file. Why should start-stop-daemon create an 
> additional pid file

start-stop-daemon will not create an additional pid file with just the
-p option. Only if you pass the -m option in addition to -p. With -p,
start-stop-daemon will only verify that the process has created the pid
file. From the start-stop-daemon manpage:

       -p, --pidfile pid-file
              Check  whether  a  process  has created the file pid-file. Note:
              using this matching option alone  might  cause  unintended  pro‐
              cesses  to  be  acted  on, if the old process terminated without
              being able to remove the pid-file.

       -m, --make-pidfile
              Used  when  starting  a program that does not create its own pid
              file. This option will make start-stop-daemon  create  the  file
              referenced  with --pidfile and place the pid into it just before
              executing the process. Note, the file will only be removed  when
              stopping  the  program  if --remove-pidfile is used.  Note: This
              feature may not work in all cases. Most notably when the program
              being  executed forks from its main process. Because of this, it
              is usually only  useful  when  combined  with  the  --background
              option.

> > Why do you pass -n ? And why do you use -a instead of -x ?
> O.k., changed -a to -x
> I thought that I need -n to be able to do a kill when shutting down the 
> server when NOT using pid file from start-stop-daemon. This was my 
> understanding from other init scripts. Am I wrong?

If you specify -p, I think doing the name-based check with -n is useless.


> >> +        killall -HUP $(basename ${DAEMON})
> > I think it's better to use the pid file here, no?
> >
> > 	   kill -HUP $(cat /var/run/slapd/slapd.pid)
> See comment above. Slapd is managing its own pid file.

And? It doesn't prevent us from using it, right?

Thanks!

Thomas
universe II Jan. 13, 2016, 5:31 p.m. UTC | #5
Thomas,
thanks for your comments. I will incorporate them, test it and send the 
patch in the next two days.

Regards,
Andreas

Am 12.01.2016 um 22:09 schrieb Thomas Petazzoni:
> Andreas,
>
> On Tue, 12 Jan 2016 22:02:28 +0100, Andreas Ehmanns wrote:
>
>> I reworked the patch and incorporated your findings. Please have a look
>> at my comments below and let me know what you think.
> Thanks! See below my comments.
>
>
>>> If you fix this, then the path to the pidfile (and argsfile) in
>>> slapd.conf are wrong, because they point to /var/run/, to which the
>>> ldap user has not write access.
>>>
>>> If you fix this again, when you start slapd, it complains:
>>>
>>> bdb_db_open: warning - no DB_CONFIG file found in
>>> directory /var/openldap-data: (2). Expect poor performance for suffix
>>> "dc=my-domain,dc=com".
>>>
>>> It should probably be fixed by using DB_CONFIG.example as DB_CONFIG
>>> in /var/openldap-data/.
>> My aim was to add the OpenLDAP server as provided by the package and
>> only make the changes necessary to allow the server to start up without
>> terminating.
>> slapd.conf is the default configuration provided by the package which is
>> a good starting point for people to setup their own configuration and
>> database. Of course everyone using the LDAP server has to make its own
>> configuration and database setup but this can't be provided or
>> preconfigured by buildroot.
> Right, but in general we try in Buildroot to provide a sane/minimal
> default configuration that "works" out of the box. It is a bit weird to
> have such a warning when the slapd daemon starts. But OK, it's not a
> very big issue either, we can always leave it as it is for now for this
> aspect.
>
>>> 	-p /var/run/slapd/slapd.pid
>> Slapd manages its own pid file. Why should start-stop-daemon create an
>> additional pid file
> start-stop-daemon will not create an additional pid file with just the
> -p option. Only if you pass the -m option in addition to -p. With -p,
> start-stop-daemon will only verify that the process has created the pid
> file. From the start-stop-daemon manpage:
>
>         -p, --pidfile pid-file
>                Check  whether  a  process  has created the file pid-file. Note:
>                using this matching option alone  might  cause  unintended  pro‐
>                cesses  to  be  acted  on, if the old process terminated without
>                being able to remove the pid-file.
>
>         -m, --make-pidfile
>                Used  when  starting  a program that does not create its own pid
>                file. This option will make start-stop-daemon  create  the  file
>                referenced  with --pidfile and place the pid into it just before
>                executing the process. Note, the file will only be removed  when
>                stopping  the  program  if --remove-pidfile is used.  Note: This
>                feature may not work in all cases. Most notably when the program
>                being  executed forks from its main process. Because of this, it
>                is usually only  useful  when  combined  with  the  --background
>                option.
>
>>> Why do you pass -n ? And why do you use -a instead of -x ?
>> O.k., changed -a to -x
>> I thought that I need -n to be able to do a kill when shutting down the
>> server when NOT using pid file from start-stop-daemon. This was my
>> understanding from other init scripts. Am I wrong?
> If you specify -p, I think doing the name-based check with -n is useless.
>
>
>>>> +        killall -HUP $(basename ${DAEMON})
>>> I think it's better to use the pid file here, no?
>>>
>>> 	   kill -HUP $(cat /var/run/slapd/slapd.pid)
>> See comment above. Slapd is managing its own pid file.
> And? It doesn't prevent us from using it, right?
>
> Thanks!
>
> Thomas
diff mbox

Patch

diff --git a/package/openldap/Config.in b/package/openldap/Config.in
index 3085a5c..30c0f65 100644
--- a/package/openldap/Config.in
+++ b/package/openldap/Config.in
@@ -6,12 +6,18 @@  config BR2_PACKAGE_OPENLDAP
 	  OpenLDAP Software is an open source implementation of
 	  the Lightweight Directory Access Protocol.
 
-	  This only installs client-side support.
+	  This only installs library support.
 
 	  http://www.openldap.org/
 
 if BR2_PACKAGE_OPENLDAP
 
+config BR2_PACKAGE_OPENLDAP_SERVER
+	bool "openldap server binary"
+	select BR2_PACKAGE_BERKELEYDB
+	help
+	  Installs the OpenLDAP server slapd
+
 config BR2_PACKAGE_OPENLDAP_CLIENTS
 	bool "openldap client binaries"
 	default y
diff --git a/package/openldap/S75slapd b/package/openldap/S75slapd
new file mode 100644
index 0000000..1365d4b
--- /dev/null
+++ b/package/openldap/S75slapd
@@ -0,0 +1,51 @@ 
+#!/bin/sh
+DAEMON=/usr/libexec/slapd
+NAME=slapd
+DESC="OpenLDAP server"
+CONF=/etc/openldap/slapd.conf
+
+ARGS="-u ldap -g ldap"
+
+set -e
+
+case "$1" in
+    start)
+        if [ ! -d /var/run/openldap ]; then
+            install -d -o ldap -g ldap -m 755 /var/run/openldap
+        fi
+
+        if [ ! -d /var/openldap-data ]; then
+            install -d -o ldap -g ldap -m 755 /var/openldap-data
+        else
+            chown -R ldap:ldap /var/openldap-data
+        fi
+
+        printf "Starting $DESC: $NAME: "
+        start-stop-daemon -S -b -n $NAME -a $DAEMON -- $ARGS
+        echo "done."
+        ;;
+    stop)
+        printf "Stopping $DESC: $NAME: "
+        start-stop-daemon -K -n $NAME
+        echo "done."
+        ;;
+    restart)
+        printf "Restarting $DESC: $NAME: "
+        $0 stop
+        $0 start
+        echo "done."
+        ;;
+    reload)
+        printf "Reloading $DESC: $NAME: "
+        killall -HUP $(basename ${DAEMON})
+        echo "done."
+        ;;
+    *)
+        echo "Usage: $0 {start|stop|restart|reload}"
+        exit 1
+        ;;
+esac
+
+exit 0
+
+
diff --git a/package/openldap/openldap.mk b/package/openldap/openldap.mk
index 17bf991..bcb285a 100644
--- a/package/openldap/openldap.mk
+++ b/package/openldap/openldap.mk
@@ -12,6 +12,17 @@  OPENLDAP_LICENSE_FILES = LICENSE
 OPENLDAP_INSTALL_STAGING = YES
 OPENLDAP_DEPENDENCIES = host-pkgconf
 
+ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),y)
+define OPENLDAP_USERS
+        ldap -1 ldap -1 * /var/run/openldap - - OpenLDAP server user
+endef
+
+define OPENLDAP_COPY_INITSCRIPT
+        $(INSTALL) -D -m 755 $(@D)/S75slapd $(TARGET_DIR)/etc/init.d/S75slapd
+endef
+OPENLDAP_POST_INSTALL_TARGET_HOOKS += OPENLDAP_COPY_INITSCRIPT
+endif
+
 ifeq ($(BR2_PACKAGE_OPENSSL),y)
 OPENLDAP_TLS = openssl
 OPENLDAP_DEPENDENCIES += openssl
@@ -44,7 +55,6 @@  OPENLDAP_CONF_ENV += ac_cv_func_memcmp_working=yes
 OPENLDAP_CONF_OPTS += \
 	--enable-syslog \
 	--disable-proctitle \
-	--disable-slapd \
 	--with-yielding-select \
 	--sysconfdir=/etc \
 	--enable-dynamic=$(if $(BR2_STATIC_LIBS),no,yes) \
@@ -52,6 +62,11 @@  OPENLDAP_CONF_OPTS += \
 	--with-mp=$(OPENLDAP_MP) \
 	CPPFLAGS="$(TARGET_CPPFLAGS) $(OPENLDAP_CPPFLAGS)"
 
+ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),)
+OPENLDAP_CONF_OPTS += \
+        --disable-slapd
+endif
+
 # Somehow, ${STRIP} does not percolates through to the shtool script
 # used to install the executables; thus, that script tries to run the
 # executable it is supposed to install, resulting in an error.