ldapd: add -r option to specify datadir path

[Landry Breuil]

Hi,

i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

Landry

Index: ldapd.8
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.8,v
retrieving revision 1.12
diff -u -r1.12 ldapd.8
--- ldapd.8 11 Aug 2014 08:21:55 -0000 1.12
+++ ldapd.8 23 Jan 2016 10:11:48 -0000
@@ -57,6 +57,11 @@
.Ar file
as the configuration file, instead of the default
.Pa /etc/ldapd.conf .
+.It Fl r Ar directory
+Store and read database files in
+.Ar directory
+, instead of the default
+.Pa /var/db/ldap .
.It Fl n
Configtest mode.
Only check the configuration file for validity.
Index: ldapd.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.c,v
retrieving revision 1.16
diff -u -r1.16 ldapd.c
--- ldapd.c 17 Jan 2016 08:13:34 -0000 1.16
+++ ldapd.c 23 Jan 2016 10:11:48 -0000
@@ -50,6 +50,7 @@

struct ldapd_stats stats;
pid_t ldape_pid;
+char * datadir;

void
usage(void)
@@ -57,7 +58,7 @@
extern char *__progname;

fprintf(stderr, "usage: %s [-dnv] [-D macro=value] "
- "[-f file] [-s file]\n", __progname);
+ "[-f file] [-r directory] [-s file]\n", __progname);
exit(1);
}

@@ -117,9 +118,10 @@
struct event ev_sigchld;
struct event ev_sighup;

+ datadir = DATADIR;
log_init(1); /* log to stderr until daemonized */

- while ((c = getopt(argc, argv, "dhvD:f:ns:")) != -1) {
+ while ((c = getopt(argc, argv, "dhvD:f:nr:s:")) != -1) {
switch (c) {
case 'd':
debug = 1;
@@ -139,6 +141,9 @@
case 'n':
configtest = 1;
break;
+ case 'r':
+ datadir = optarg;
+ break;
case 's':
csockpath = optarg;
break;
@@ -366,7 +371,7 @@
/* make sure path is null-terminated */
oreq->path[PATH_MAX] = '\0';

- if (strncmp(oreq->path, DATADIR, strlen(DATADIR)) != 0) {
+ if (strncmp(oreq->path, datadir, strlen(datadir)) != 0) {
log_warnx("refusing to open file %s", oreq->path);
fatal("ldape sent invalid open request");
}
Index: namespace.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/namespace.c,v
retrieving revision 1.14
diff -u -r1.14 namespace.c
--- namespace.c 24 Dec 2015 17:47:57 -0000 1.14
+++ namespace.c 23 Jan 2016 10:11:48 -0000
@@ -38,6 +38,7 @@
static int namespace_set_fd(struct namespace *ns,
struct btree **bt, int fd, unsigned int flags);

+extern char *datadir;
int
namespace_begin_txn(struct namespace *ns, struct btree_txn **data_txn,
struct btree_txn **indx_txn, int rdonly)
@@ -115,7 +116,7 @@
if (ns->sync == 0)
db_flags |= BT_NOSYNC;

- if (asprintf(&ns->data_path, "%s/%s_data.db", DATADIR, ns->suffix) < 0)
+ if (asprintf(&ns->data_path, "%s/%s_data.db", datadir, ns->suffix) < 0)
return -1;
log_info("opening namespace %s", ns->suffix);
ns->data_db = btree_open(ns->data_path, db_flags | BT_REVERSEKEY, 0644);
@@ -124,7 +125,7 @@

btree_set_cache_size(ns->data_db, ns->cache_size);

- if (asprintf(&ns->indx_path, "%s/%s_indx.db", DATADIR, ns->suffix) < 0)
+ if (asprintf(&ns->indx_path, "%s/%s_indx.db", datadir, ns->suffix) < 0)
return -1;
ns->indx_db = btree_open(ns->indx_path, db_flags, 0644);
if (ns->indx_db == NULL)

[Sebastien Marie]

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !
Index: ldapd.8
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.8,v
retrieving revision 1.12
diff -u -r1.12 ldapd.8
--- ldapd.8 11 Aug 2014 08:21:55 -0000 1.12
+++ ldapd.8 23 Jan 2016 10:11:48 -0000
@@ -57,6 +57,11 @@
.Ar file
as the configuration file, instead of the default
.Pa /etc/ldapd.conf .
+.It Fl r Ar directory
+Store and read database files in
+.Ar directory
+, instead of the default
+.Pa /var/db/ldap .
.It Fl n
Configtest mode.
Only check the configuration file for validity.
Index: ldapd.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.c,v
retrieving revision 1.16
diff -u -r1.16 ldapd.c
--- ldapd.c 17 Jan 2016 08:13:34 -0000 1.16
+++ ldapd.c 23 Jan 2016 10:11:48 -0000
@@ -50,6 +50,7 @@
struct ldapd_stats stats;
pid_t ldape_pid;
+char * datadir;
void
usage(void)
@@ -57,7 +58,7 @@
extern char *__progname;
fprintf(stderr, "usage: %s [-dnv] [-D macro=value] "
- "[-f file] [-s file]\n", __progname);
+ "[-f file] [-r directory] [-s file]\n", __progname);
exit(1);
}
@@ -117,9 +118,10 @@
struct event ev_sigchld;
struct event ev_sighup;
+ datadir = DATADIR;
log_init(1); /* log to stderr until daemonized */
- while ((c = getopt(argc, argv, "dhvD:f:ns:")) != -1) {
+ while ((c = getopt(argc, argv, "dhvD:f:nr:s:")) != -1) {
switch (c) {
debug = 1;
@@ -139,6 +141,9 @@
configtest = 1;
break;
+ datadir = optarg;
+ break;
csockpath = optarg;
break;
@@ -366,7 +371,7 @@
/* make sure path is null-terminated */
oreq->path[PATH_MAX] = '\0';
- if (strncmp(oreq->path, DATADIR, strlen(DATADIR)) != 0) {
+ if (strncmp(oreq->path, datadir, strlen(datadir)) != 0) {
log_warnx("refusing to open file %s", oreq->path);
fatal("ldape sent invalid open request");
}
Index: namespace.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/namespace.c,v
retrieving revision 1.14
diff -u -r1.14 namespace.c
--- namespace.c 24 Dec 2015 17:47:57 -0000 1.14
+++ namespace.c 23 Jan 2016 10:11:48 -0000
@@ -38,6 +38,7 @@
static int namespace_set_fd(struct namespace *ns,
struct btree **bt, int fd, unsigned int flags);
+extern char *datadir;
int
namespace_begin_txn(struct namespace *ns, struct btree_txn **data_txn,
struct btree_txn **indx_txn, int rdonly)
@@ -115,7 +116,7 @@
if (ns->sync == 0)
db_flags |= BT_NOSYNC;
- if (asprintf(&ns->data_path, "%s/%s_data.db", DATADIR, ns->suffix) < 0)
+ if (asprintf(&ns->data_path, "%s/%s_data.db", datadir, ns->suffix) < 0)
return -1;
log_info("opening namespace %s", ns->suffix);
ns->data_db = btree_open(ns->data_path, db_flags | BT_REVERSEKEY, 0644);
@@ -124,7 +125,7 @@
btree_set_cache_size(ns->data_db, ns->cache_size);
- if (asprintf(&ns->indx_path, "%s/%s_indx.db", DATADIR, ns->suffix) < 0)
+ if (asprintf(&ns->indx_path, "%s/%s_indx.db", datadir, ns->suffix) < 0)
return -1;
ns->indx_db = btree_open(ns->indx_path, db_flags, 0644);
if (ns->indx_db == NULL)

[Jérémie Courrèges-Anglas]

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

I have smallish tweaks to propose but nothing that prevents this diff to
go in as is. ok jca@

[Jérémie Courrèges-Anglas]

Post by Jérémie Courrèges-Anglas

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

I have smallish tweaks to propose but nothing that prevents this diff to

Since both ldapd and ldapctl may access the db, ldapctl should be
modified similarly.

Comments/ok?

Index: ldapctl.8
===================================================================
RCS file: /cvs/src/usr.sbin/ldapctl/ldapctl.8,v
retrieving revision 1.4
diff -u -p -r1.4 ldapctl.8
--- ldapctl.8 21 Jul 2010 06:32:14 -0000 1.4
+++ ldapctl.8 1 Feb 2016 17:57:55 -0000
@@ -24,6 +24,7 @@
.Nm ldapctl
.Op Fl v
.Op Fl f Ar file
+.Op Fl r Ar datadir
.Op Fl s Ar socket
.Ar command
.Op Ar argument ...
@@ -41,6 +42,11 @@ Use
.Ar file
as the configuration file, instead of the default
.Pa /etc/ldapd.conf .
+.It Fl r Ar directory
+Store and read database files in
+.Ar directory
+, instead of the default
+.Pa /var/db/ldap .
.It Fl s Ar socket
Use
.Ar socket
Index: ldapctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapctl/ldapctl.c,v
retrieving revision 1.7
diff -u -p -r1.7 ldapctl.c
--- ldapctl.c 5 Dec 2015 13:19:13 -0000 1.7
+++ ldapctl.c 1 Feb 2016 17:57:55 -0000
@@ -56,10 +56,10 @@ void show_stats(struct imsg *imsg);
void show_dbstats(const char *prefix, struct btree_stat *st);
void show_nsstats(struct imsg *imsg);
int compact_db(const char *path);
-int compact_namespace(struct namespace *ns);
-int compact_namespaces(void);
-int index_namespace(struct namespace *ns);
-int index_namespaces(void);
+int compact_namespace(struct namespace *ns, const char *datadir);
+int compact_namespaces(const char *datadir);
+int index_namespace(struct namespace *ns, const char *datadir);
+int index_namespaces(const char *datadir);

__dead void
usage(void)
@@ -67,7 +67,8 @@ usage(void)
extern char *__progname;

fprintf(stderr,
- "usage: %s [-v] [-f file] [-s socket] command [argument ...]\n",
+ "usage: %s [-v] [-f file] [-r directory] [-s socket] "
+ "command [argument ...]\n",
__progname);
exit(1);
}
@@ -93,11 +94,11 @@ compact_db(const char *path)
}

int
-compact_namespace(struct namespace *ns)
+compact_namespace(struct namespace *ns, const char *datadir)
{
char *path;

- if (asprintf(&path, "%s/%s_data.db", DATADIR, ns->suffix) < 0)
+ if (asprintf(&path, "%s/%s_data.db", datadir, ns->suffix) < 0)
return -1;
if (compact_db(path) != 0) {
log_warn("%s", path);
@@ -106,7 +107,7 @@ compact_namespace(struct namespace *ns)
}
free(path);

- if (asprintf(&path, "%s/%s_indx.db", DATADIR, ns->suffix) < 0)
+ if (asprintf(&path, "%s/%s_indx.db", datadir, ns->suffix) < 0)
return -1;
if (compact_db(path) != 0) {
log_warn("%s", path);
@@ -119,19 +120,22 @@ compact_namespace(struct namespace *ns)
}

int
-compact_namespaces(void)
+compact_namespaces(const char *datadir)
{
struct namespace *ns;

- TAILQ_FOREACH(ns, &conf->namespaces, next)
- if (SLIST_EMPTY(&ns->referrals) && compact_namespace(ns) != 0)
+ TAILQ_FOREACH(ns, &conf->namespaces, next) {
+ if (SLIST_EMPTY(&ns->referrals))
+ continue;
+ if (compact_namespace(ns, datadir) != 0)
return -1;
+ }

return 0;
}

int
-index_namespace(struct namespace *ns)
+index_namespace(struct namespace *ns, const char *datadir)
{
struct btval key, val;
struct btree *data_db, *indx_db;
@@ -150,7 +154,7 @@ index_namespace(struct namespace *ns)
if (data_db == NULL)
return -1;

- if (asprintf(&path, "%s/%s_indx.db", DATADIR, ns->suffix) < 0)
+ if (asprintf(&path, "%s/%s_indx.db", datadir, ns->suffix) < 0)
return -1;
indx_db = btree_open(path, BT_NOSYNC, 0644);
free(path);
@@ -212,13 +216,16 @@ index_namespace(struct namespace *ns)
}

int
-index_namespaces(void)
+index_namespaces(const char *datadir)
{
struct namespace *ns;

- TAILQ_FOREACH(ns, &conf->namespaces, next)
- if (SLIST_EMPTY(&ns->referrals) && index_namespace(ns) != 0)
+ TAILQ_FOREACH(ns, &conf->namespaces, next) {
+ if (SLIST_EMPTY(&ns->referrals))
+ continue;
+ if (index_namespace(ns, datadir) != 0)
return -1;
+ }

return 0;
}
@@ -237,6 +244,7 @@ main(int argc, char *argv[])
ssize_t n;
int ch;
enum action action = NONE;
+ const char *datadir = DATADIR;
const char *sock = LDAPD_SOCKET;
char *conffile = CONFFILE;
struct sockaddr_un sun;
@@ -245,11 +253,14 @@ main(int argc, char *argv[])

log_init(1);

- while ((ch = getopt(argc, argv, "f:s:v")) != -1) {
+ while ((ch = getopt(argc, argv, "f:r:s:v")) != -1) {
switch (ch) {
case 'f':
conffile = optarg;
break;
+ case 'r':
+ datadir = optarg;
+ break;
case 's':
sock = optarg;
break;
@@ -295,9 +306,9 @@ main(int argc, char *argv[])
err(1, "pledge");

if (action == COMPACT_DB)
- return compact_namespaces();
+ return compact_namespaces(datadir);
else
- return index_namespaces();
+ return index_namespaces(datadir);
}

/* connect to ldapd control socket */

[Landry Breuil]

Post by Jérémie Courrèges-Anglas

Post by Jérémie Courrèges-Anglas

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

I have smallish tweaks to propose but nothing that prevents this diff to

Since both ldapd and ldapctl may access the db, ldapctl should be
modified similarly.

Mh, i have to admit i thought ldapctl was sending imsgs commands to
ldapd to do the actual compact/reindexing instead of doing it offline..
so yes this makes sense, just add the chdir() check as gsoares@ proposed
for ldapd.

Landry

[Jérémie Courrèges-Anglas]

Post by Jérémie Courrèges-Anglas

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

I have smallish tweaks to propose but nothing that prevents this diff to

The tweaks that I had in mind:
- the string pointed to by datadir should not be modified
- we can initialize datadir at compile time
- in namespace.c move the datadir decl above local decls

Index: ldapd.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.c,v
retrieving revision 1.18
diff -u -p -r1.18 ldapd.c
--- ldapd.c 2 Feb 2016 14:59:20 -0000 1.18
+++ ldapd.c 2 Feb 2016 18:22:12 -0000
@@ -51,7 +51,7 @@ static void ldapd_cleanup(char *);

struct ldapd_stats stats;
pid_t ldape_pid;
-char * datadir;
+const char *datadir = DATADIR;

void
usage(void)
@@ -120,7 +120,6 @@ main(int argc, char *argv[])
struct event ev_sighup;
struct stat sb;

- datadir = DATADIR;
log_init(1); /* log to stderr until daemonized */

while ((c = getopt(argc, argv, "dhvD:f:nr:s:")) != -1) {
Index: namespace.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/namespace.c,v
retrieving revision 1.15
diff -u -p -r1.15 namespace.c
--- namespace.c 1 Feb 2016 20:00:18 -0000 1.15
+++ namespace.c 2 Feb 2016 18:22:12 -0000
@@ -28,6 +28,8 @@

#include "ldapd.h"

+extern const char *datadir;
+
/* Maximum number of requests to queue per namespace during compaction.
* After this many requests, we return LDAP_BUSY.
*/
@@ -38,7 +40,6 @@ static void namespace_queue_replay(int
static int namespace_set_fd(struct namespace *ns,
struct btree **bt, int fd, unsigned int flags);

-extern char *datadir;
int
namespace_begin_txn(struct namespace *ns, struct btree_txn **data_txn,
struct btree_txn **indx_txn, int rdonly)

[Mark Kettenis]

Date: Tue, 02 Feb 2016 19:23:25 +0100

Post by Jérémie Courrèges-Anglas

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

I have smallish tweaks to propose but nothing that prevents this diff to

- the string pointed to by datadir should not be modified
- we can initialize datadir at compile time
- in namespace.c move the datadir decl above local decls

Makes sense to me.

Index: ldapd.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.c,v
retrieving revision 1.18
diff -u -p -r1.18 ldapd.c
--- ldapd.c 2 Feb 2016 14:59:20 -0000 1.18
+++ ldapd.c 2 Feb 2016 18:22:12 -0000
@@ -51,7 +51,7 @@ static void ldapd_cleanup(char *);
struct ldapd_stats stats;
pid_t ldape_pid;
-char * datadir;
+const char *datadir = DATADIR;
void
usage(void)
@@ -120,7 +120,6 @@ main(int argc, char *argv[])
struct event ev_sighup;
struct stat sb;
- datadir = DATADIR;
log_init(1); /* log to stderr until daemonized */
while ((c = getopt(argc, argv, "dhvD:f:nr:s:")) != -1) {
Index: namespace.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/namespace.c,v
retrieving revision 1.15
diff -u -p -r1.15 namespace.c
--- namespace.c 1 Feb 2016 20:00:18 -0000 1.15
+++ namespace.c 2 Feb 2016 18:22:12 -0000
@@ -28,6 +28,8 @@
#include "ldapd.h"
+extern const char *datadir;
+
/* Maximum number of requests to queue per namespace during compaction.
* After this many requests, we return LDAP_BUSY.
*/
@@ -38,7 +40,6 @@ static void namespace_queue_replay(int
static int namespace_set_fd(struct namespace *ns,
struct btree **bt, int fd, unsigned int flags);
-extern char *datadir;
int
namespace_begin_txn(struct namespace *ns, struct btree_txn **data_txn,
struct btree_txn **indx_txn, int rdonly)
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE

[Gleydson Soares]

Hi Landry,

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

slight tweak,
looks like it is missing a chdir(3) to check failure if an invalid(nonexistent)
datadir was passed to optarg.

I just added these lines in ldapd.c:
159 if (datadir && chdir(datadir))
160 err(1, "chdir");

% doas ./ldapd -r /home/gsoares/non-existentdoas
ldapd: chdir: No such file or directory
%

updated diff attached.

[Jérémie Courrèges-Anglas]

Post by Gleydson Soares
Hi Landry,

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

slight tweak,
looks like it is missing a chdir(3) to check failure if an invalid(nonexistent)
datadir was passed to optarg.
159 if (datadir && chdir(datadir))
160 err(1, "chdir");

Hum, while a check would be nicer, I prefer when daemons stick to /.

[...]

[Landry Breuil]

Post by Jérémie Courrèges-Anglas

Post by Gleydson Soares
Hi Landry,

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

slight tweak,
looks like it is missing a chdir(3) to check failure if an invalid(nonexistent)
datadir was passed to optarg.
159 if (datadir && chdir(datadir))
160 err(1, "chdir");

Hum, while a check would be nicer, I prefer when daemons stick to /.

Thinking about it .. would a call to access(2) with R_OK|W_OK|R_OK|F_OK satisfy
everyone ? Or only F_OK ?

Landry

[Gleydson Soares]

Post by Landry Breuil
Thinking about it .. would a call to access(2) with R_OK|W_OK|R_OK|F_OK satisfy
everyone ? Or only F_OK ?

Sounds better than chdir(2), but it will lack if datadir was passed to access(2)
without including trailing "/"

eg with access(datadir, F_OK):
$ touch /home/gsoares/testfile <- creating a file, not a directory
$ doas ./ldapd -r /home/gsoares/testfile ; echo $? <- no trailing /home/gsoares/testfile"/"
0

so for directory existence check, I would suggest to use just stat(2). diff attached.

[Jérémie Courrèges-Anglas]

Post by Gleydson Soares

Post by Landry Breuil
Thinking about it .. would a call to access(2) with R_OK|W_OK|R_OK|F_OK satisfy
everyone ? Or only F_OK ?

Sounds better than chdir(2), but it will lack if datadir was passed to access(2)
without including trailing "/"
$ touch /home/gsoares/testfile <- creating a file, not a directory
$ doas ./ldapd -r /home/gsoares/testfile ; echo $? <- no trailing /home/gsoares/testfile"/"
0
so for directory existence check, I would suggest to use just stat(2). diff attached.
Index: ldapd.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.c,v
retrieving revision 1.17
diff -u -p -r1.17 ldapd.c
--- ldapd.c 1 Feb 2016 20:00:18 -0000 1.17
+++ ldapd.c 2 Feb 2016 10:10:43 -0000
@@ -17,6 +17,7 @@
*/
#include <sys/queue.h>
+#include <sys/stat.h>
#include <sys/un.h>
#include <sys/types.h>
#include <sys/wait.h>
@@ -117,6 +118,7 @@ main(int argc, char *argv[])
struct event ev_sigterm;
struct event ev_sigchld;
struct event ev_sighup;
+ struct stat s;

"s" sounds like a socket to me, what about "sb"?

Post by Gleydson Soares
datadir = DATADIR;
log_init(1); /* log to stderr until daemonized */
@@ -178,8 +180,8 @@ main(int argc, char *argv[])
skip_chroot = 1;
}
- if (datadir && chdir(datadir))
- err(1, "chdir");
+ if ((stat(datadir, &s) == -1) || !S_ISDIR(s.st_mode))
+ errx(1, "Invalid directory");

With such a diagnostic you don't get much detail about the exact
problem.

Post by Gleydson Soares
if (!skip_chroot && (pw = getpwnam(LDAPD_USER)) == NULL)
err(1, "%s", LDAPD_USER);

Here's a similar diff for ldapctl, thoughts?

Index: ldapctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapctl/ldapctl.c,v
retrieving revision 1.8
diff -u -p -r1.8 ldapctl.c
--- ldapctl.c 2 Feb 2016 12:47:10 -0000 1.8
+++ ldapctl.c 2 Feb 2016 12:54:12 -0000
@@ -22,6 +22,7 @@

#include <sys/types.h>
#include <sys/socket.h>
+#include <sys/stat.h>
#include <sys/queue.h>
#include <sys/un.h>
#include <sys/tree.h>
@@ -245,6 +246,7 @@ main(int argc, char *argv[])
int ch;
enum action action = NONE;
const char *datadir = DATADIR;
+ struct stat sb;
const char *sock = LDAPD_SOCKET;
char *conffile = CONFFILE;
struct sockaddr_un sun;
@@ -277,6 +279,11 @@ main(int argc, char *argv[])

if (argc == 0)
usage();
+
+ if (stat(datadir, &sb) == -1)
+ err(1, "%s", datadir);
+ if (!S_ISDIR(sb.st_mode))
+ errx(1, "%s is not a directory", datadir);

log_verbose(verbose);

[Landry Breuil]

Post by Jérémie Courrèges-Anglas

Post by Gleydson Soares

Post by Landry Breuil
Thinking about it .. would a call to access(2) with R_OK|W_OK|R_OK|F_OK satisfy
everyone ? Or only F_OK ?

Sounds better than chdir(2), but it will lack if datadir was passed to access(2)
without including trailing "/"
$ touch /home/gsoares/testfile <- creating a file, not a directory
$ doas ./ldapd -r /home/gsoares/testfile ; echo $? <- no trailing /home/gsoares/testfile"/"
0
so for directory existence check, I would suggest to use just stat(2). diff attached.
Index: ldapd.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.c,v
retrieving revision 1.17
diff -u -p -r1.17 ldapd.c
--- ldapd.c 1 Feb 2016 20:00:18 -0000 1.17
+++ ldapd.c 2 Feb 2016 10:10:43 -0000
@@ -17,6 +17,7 @@
*/
#include <sys/queue.h>
+#include <sys/stat.h>
#include <sys/un.h>
#include <sys/types.h>
#include <sys/wait.h>
@@ -117,6 +118,7 @@ main(int argc, char *argv[])
struct event ev_sigterm;
struct event ev_sigchld;
struct event ev_sighup;
+ struct stat s;

"s" sounds like a socket to me, what about "sb"?

Post by Gleydson Soares
datadir = DATADIR;
log_init(1); /* log to stderr until daemonized */
@@ -178,8 +180,8 @@ main(int argc, char *argv[])
skip_chroot = 1;
}
- if (datadir && chdir(datadir))
- err(1, "chdir");
+ if ((stat(datadir, &s) == -1) || !S_ISDIR(s.st_mode))
+ errx(1, "Invalid directory");

With such a diagnostic you don't get much detail about the exact
problem.

Post by Gleydson Soares
if (!skip_chroot && (pw = getpwnam(LDAPD_USER)) == NULL)
err(1, "%s", LDAPD_USER);

Here's a similar diff for ldapctl, thoughts?

I like this one better,okay.
Make sure ldapd and ldapctl are aligned..

Landry

[Gleydson Soares]

Post by Jérémie Courrèges-Anglas
Here's a similar diff for ldapctl, thoughts?

With your tweaks sounds much better.
OK gsoares@

[Gleydson Soares]

Post by Jérémie Courrèges-Anglas

Post by Gleydson Soares
Hi Landry,

Post by Landry Breuil
Hi,
i'm tinkering with ldapd and writing regress tests for it, and to
allow running independent instances (with separate port/control
socket/etc) i needed to add the possibility to specify an alternative
datadir, which was so far #defined in the code.
Patch is pretty simple and works fine, i'm open to suggestions of course
on a better wording for the manpage and option choose (i went for -r..)
okays welcome too !

slight tweak,
looks like it is missing a chdir(3) to check failure if an invalid(nonexistent)
datadir was passed to optarg.
159 if (datadir && chdir(datadir))
160 err(1, "chdir");

Hum, while a check would be nicer, I prefer when daemons stick to /.

It is. ldape() will take care right afterwards.