From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
To: buildroot@busybox.net
Subject: [Buildroot] Move openldap package and enable LDAP server?
Date: Sat, 12 Dec 2015 14:01:45 +0100 [thread overview]
Message-ID: <20151212140145.5a7583dc@free-electrons.com> (raw)
In-Reply-To: <5669CC92.8080504@gmx.de>
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
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
next prev parent reply other threads:[~2015-12-12 13:01 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-04 8:13 [Buildroot] Move openldap package and enable LDAP server? Andreas Ehmanns
2015-12-04 8:19 ` Thomas Petazzoni
2015-12-04 8:21 ` Andreas Ehmanns
2015-12-10 19:03 ` Andreas Ehmanns
2015-12-12 13:01 ` Thomas Petazzoni [this message]
2015-12-12 17:20 ` Andreas Ehmanns
2015-12-17 20:43 ` Andreas Ehmanns
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151212140145.5a7583dc@free-electrons.com \
--to=thomas.petazzoni@free-electrons.com \
--cc=buildroot@busybox.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.