diff mbox

Move openldap package and enable LDAP server?

Message ID 5669CC92.8080504@gmx.de
State Changes Requested
Headers show

Commit Message

universe II Dec. 10, 2015, 7:03 p.m. UTC 
Am 04.12.2015 um 09:21 schrieb Andreas Ehmanns:
> Am 04.12.2015 um 09:19 schrieb Thomas Petazzoni:
>> Hello,
>>
>> On Fri, 4 Dec 2015 09:13:12 +0100, Andreas Ehmanns wrote:
>>
>>> I was going to use openLDAP on my embedded device and found the
>>> configuration under Target Packages -> Libraries -> Networking.
>>> You can enable openldap which only generates the ldap libraries but 
>>> have
>>> an option to let the client applications being built.
>>> Since I need the LDAP server I added an option to compile the LDAP
>>> server too. Everything works fine and I thought about submitting the
>>> patch but then I asked me if the configuration location is still the
>>> right place.
>>> Wouldn't it be better to place the openldap configuration under
>>> Target Packages -> Networking Applications
>>> instead of
>>> Target Packages -> Libraries -> Networking
>>>
>>> What is your opinion?
>> Yes, I agree that it probably makes more sense to have OpenLDAP in
>> Networking Applications, even if it indeed also provides a client
>> library.
>>
>> Thomas
> O.k., I will prepare/propose a patch for adding the ldap server option 
> and moving the configuration to Networking Applications.
>
> Regards,
> Andreas

Thomas,
please find attached the patch for adding the openldap server. It 
contains the server option, moves the configuration to "Networking 
Applications" and adds an init script for the ldap server.

Please let me know what you think about the patch and if there are 
things that I should change.

Regards,
Andreas

> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Comments

Thomas Petazzoni Dec. 12, 2015, 1:01 p.m. UTC | #1 
Hello Andreas,

On Thu, 10 Dec 2015 20:03:46 +0100, Andreas Ehmanns wrote:

> please find attached the patch for adding the openldap server. It 
> contains the server option, moves the configuration to "Networking 
> Applications" and adds an init script for the ldap server.
> 
> Please let me know what you think about the patch and if there are 
> things that I should change.

Thanks. But could you please send the patch with 'git send-email'
rather than as an attachment? It allows the patch to be reviewed
properly, and also easily applied.

A few comments though:

 * The commit title is too long. Something like:

	openldap: add support to build the server

   would be preferable.

 * I think you should keep the "default y" on
   BR2_PACKAGE_OPENLDAP_CLIENTS in order to preserve the existing
   behavior, and not breaking things for current users.

 * In the S75sldapd script, I believe checking for the presence of the
   daemon and config file is not really needed.

   If the -4 in ARGS is to force IPv4 only, I believe it's not really
   great. Why not also support IPv6 ?

 * There is a fix of tabs and spaces for the indentation in the shell
   script, please try to be consistent.

 * Please use "printf" rather than "echo -n" in the shell script, since
   printf is POSIX, while "echo -n" is not.

 * We normally use a PID file, see S50dropbear for example. Is slapd
   already managing its own PID file ?

 * In the .mk file, the OPENLDAP_USERS definition is inside a condition
   that is commented out, it doesn't look good.

 * Don't do ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),n), but just ifeq
   ($(BR2_PACKAGE_OPENLDAP_SERVER),). An option will never have the
   value 'n': when an option is disabled, its value is the empty string.

Could you take those comments into account and send an updated version?

Thanks a lot!

Thomas
universe II Dec. 12, 2015, 5:20 p.m. UTC | #2 
Hi Thomas,

Am 12.12.2015 um 14:01 schrieb Thomas Petazzoni:
> Hello Andreas,
>
> On Thu, 10 Dec 2015 20:03:46 +0100, Andreas Ehmanns wrote:
>
>> please find attached the patch for adding the openldap server. It
>> contains the server option, moves the configuration to "Networking
>> Applications" and adds an init script for the ldap server.
>>
>> Please let me know what you think about the patch and if there are
>> things that I should change.
> Thanks. But could you please send the patch with 'git send-email'
> rather than as an attachment? It allows the patch to be reviewed
> properly, and also easily applied.
>
> A few comments though:
>
>   * The commit title is too long. Something like:
>
> 	openldap: add support to build the server
>
>     would be preferable.
>
>   * I think you should keep the "default y" on
>     BR2_PACKAGE_OPENLDAP_CLIENTS in order to preserve the existing
>     behavior, and not breaking things for current users.
>
>   * In the S75sldapd script, I believe checking for the presence of the
>     daemon and config file is not really needed.
>
>     If the -4 in ARGS is to force IPv4 only, I believe it's not really
>     great. Why not also support IPv6 ?
>
>   * There is a fix of tabs and spaces for the indentation in the shell
>     script, please try to be consistent.
>
>   * Please use "printf" rather than "echo -n" in the shell script, since
>     printf is POSIX, while "echo -n" is not.
>
>   * We normally use a PID file, see S50dropbear for example. Is slapd
>     already managing its own PID file ?
>
>   * In the .mk file, the OPENLDAP_USERS definition is inside a condition
>     that is commented out, it doesn't look good.
>
>   * Don't do ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),n), but just ifeq
>     ($(BR2_PACKAGE_OPENLDAP_SERVER),). An option will never have the
>     value 'n': when an option is disabled, its value is the empty string.
>
> Could you take those comments into account and send an updated version?
>
> Thanks a lot!
>
> Thomas
thanks for your comments. I will incorporate the changes next week and 
send a patch using git.

Regards,
Andreas
universe II Dec. 17, 2015, 8:43 p.m. UTC | #3 
Thomas, All,

Am 12.12.2015 um 14:01 schrieb Thomas Petazzoni:
> Hello Andreas,
>
> On Thu, 10 Dec 2015 20:03:46 +0100, Andreas Ehmanns wrote:
>
>> please find attached the patch for adding the openldap server. It
>> contains the server option, moves the configuration to "Networking
>> Applications" and adds an init script for the ldap server.
>>
>> Please let me know what you think about the patch and if there are
>> things that I should change.
> Thanks. But could you please send the patch with 'git send-email'
> rather than as an attachment? It allows the patch to be reviewed
> properly, and also easily applied.
>
> A few comments though:
>
>   * The commit title is too long. Something like:
>
> 	openldap: add support to build the server
>
>     would be preferable.
>
>   * I think you should keep the "default y" on
>     BR2_PACKAGE_OPENLDAP_CLIENTS in order to preserve the existing
>     behavior, and not breaking things for current users.
>
>   * In the S75sldapd script, I believe checking for the presence of the
>     daemon and config file is not really needed.
>
>     If the -4 in ARGS is to force IPv4 only, I believe it's not really
>     great. Why not also support IPv6 ?
>
>   * There is a fix of tabs and spaces for the indentation in the shell
>     script, please try to be consistent.
>
>   * Please use "printf" rather than "echo -n" in the shell script, since
>     printf is POSIX, while "echo -n" is not.
>
>   * We normally use a PID file, see S50dropbear for example. Is slapd
>     already managing its own PID file ?
>
>   * In the .mk file, the OPENLDAP_USERS definition is inside a condition
>     that is commented out, it doesn't look good.
>
>   * Don't do ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),n), but just ifeq
>     ($(BR2_PACKAGE_OPENLDAP_SERVER),). An option will never have the
>     value 'n': when an option is disabled, its value is the empty string.
>
> Could you take those comments into account and send an updated version?
>
> Thanks a lot!
>
> Thomas
I incorporated your proposed changes and re-sent the patch using git.
Note: slapd is managing it's own PID file. So there was no need to add 
this to the init script.

Regards,
Andreas
diff mbox

Patch

From 12d2719ee1be07c18144cf2990cccf22abd0cc6b Mon Sep 17 00:00:00 2001
From: Andreas Ehmanns <universeii@gmx.de>
Date: Thu, 10 Dec 2015 16:56:34 +0100
Subject: [PATCH 2/2] Added server to openldap package and changed package
 location to Networking Applications

Signed-off-by: Andreas Ehmanns <universeii@gmx.de>
---
 package/Config.in            |  2 +-
 package/openldap/Config.in   |  9 ++++++--
 package/openldap/S75slapd    | 54 ++++++++++++++++++++++++++++++++++++++++++++
 package/openldap/openldap.mk | 12 +++++++++-
 4 files changed, 73 insertions(+), 4 deletions(-)
 create mode 100755 package/openldap/S75slapd

diff --git a/package/Config.in b/package/Config.in
index 2bdad01..a42a568 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -1059,7 +1059,6 @@  menu "Networking"
 	source "package/nss-mdns/Config.in"
 	source "package/nss-pam-ldapd/Config.in"
 	source "package/omniorb/Config.in"
-	source "package/openldap/Config.in"
 	source "package/openpgm/Config.in"
 	source "package/ortp/Config.in"
 	source "package/qdecoder/Config.in"
@@ -1346,6 +1345,7 @@  endif
 	source "package/odhcploc/Config.in"
 	source "package/olsr/Config.in"
 	source "package/open-plc-utils/Config.in"
+	source "package/openldap/Config.in"
 	source "package/openntpd/Config.in"
 	source "package/openobex/Config.in"
 	source "package/openssh/Config.in"
diff --git a/package/openldap/Config.in b/package/openldap/Config.in
index 3085a5c..4c5e903 100644
--- a/package/openldap/Config.in
+++ b/package/openldap/Config.in
@@ -6,15 +6,20 @@  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
 	help
 	  Install the OpenLDAP client tools (ldapadd, ldapcompare, ldapdelete,
 	  ldapexop, ldapmodify, ldapmodrdn, ldappasswd, ldapsearch, ldapurl,
diff --git a/package/openldap/S75slapd b/package/openldap/S75slapd
new file mode 100755
index 0000000..e358a84
--- /dev/null
+++ b/package/openldap/S75slapd
@@ -0,0 +1,54 @@ 
+#!/bin/sh
+DAEMON=/usr/libexec/slapd
+NAME=slapd
+DESC="OpenLDAP server"
+CONF=/etc/openldap/slapd.conf
+
+test -f $DAEMON || exit 1
+test -r $CONF || exit 2
+
+ARGS="-4 -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
+
+	echo -n "Starting $DESC: $NAME: "
+	start-stop-daemon -S -b -n $NAME -a $DAEMON -- $ARGS
+        echo "done."
+        ;;
+    stop)
+        echo -n "Stopping $DESC: $NAME: "
+        start-stop-daemon -K -n $NAME
+        echo "done."
+        ;;
+    restart)
+        echo "Restarting $DESC: $NAME: "
+        $0 stop
+        $0 start
+        echo "done."
+        ;;
+    reload)
+        echo -n "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..725666c 100644
--- a/package/openldap/openldap.mk
+++ b/package/openldap/openldap.mk
@@ -12,6 +12,12 @@  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
+#endif
+
 ifeq ($(BR2_PACKAGE_OPENSSL),y)
 OPENLDAP_TLS = openssl
 OPENLDAP_DEPENDENCIES += openssl
@@ -44,7 +50,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 +57,11 @@  OPENLDAP_CONF_OPTS += \
 	--with-mp=$(OPENLDAP_MP) \
 	CPPFLAGS="$(TARGET_CPPFLAGS) $(OPENLDAP_CPPFLAGS)"
 
+ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),n)
+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.
-- 
2.1.2