Re: ldapd: add -r option to specify datadir path

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

j...@wxcvbn.org (Jérémie Courrèges-Anglas) writes:

> Sebastien Marie  writes:
>
>> On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
>>> 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 !
>>> 
>>
>> the diff looks good and it would permit to make regress tests, so OK 
>> semarie@ :)
>
> I have smallish tweaks to propose but nothing that prevents this diff to
> go in as is.  ok jca@

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 -   1.18
+++ ldapd.c 2 Feb 2016 18:22:12 -
@@ -51,7 +51,7 @@ static voidldapd_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 -   1.15
+++ namespace.c 2 Feb 2016 18:22:12 -
@@ -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 voidnamespace_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

Re: ldapd: add -r option to specify datadir path

[Mark Kettenis]

> From: j...@wxcvbn.org (=?utf-8?Q?J=C3=A9r=C3=A9mie_Courr=C3=A8ges-Anglas?=)
> Date: Tue, 02 Feb 2016 19:23:25 +0100
> 
> j...@wxcvbn.org (Jérémie Courrèges-Anglas) writes:
> 
> > Sebastien Marie  writes:
> >
> >> On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
> >>> 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 !
> >>> 
> >>
> >> the diff looks good and it would permit to make regress tests, so OK 
> >> semarie@ :)
> >
> > I have smallish tweaks to propose but nothing that prevents this diff to
> > go in as is.  ok jca@
> 
> 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

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 -   1.18
> +++ ldapd.c   2 Feb 2016 18:22:12 -
> @@ -51,7 +51,7 @@ static void  ldapd_cleanup(char *);
>  
>  struct ldapd_statsstats;
>  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 -   1.15
> +++ namespace.c   2 Feb 2016 18:22:12 -
> @@ -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 intnamespace_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
> 
> 
> 

Re: ldapd: add -r option to specify datadir path

[Gleydson Soares]

> Here's a similar diff for ldapctl, thoughts?

With your tweaks sounds much better.
OK gsoares@

Re: ldapd: add -r option to specify datadir path

[Gleydson Soares]

> 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.
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 -   1.17
+++ ldapd.c 2 Feb 2016 10:10:43 -
@@ -17,6 +17,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -117,6 +118,7 @@ main(int argc, char *argv[])
struct event ev_sigterm;
struct event ev_sigchld;
struct event ev_sighup;
+   struct stat  s;
 
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, ) == -1) || !S_ISDIR(s.st_mode))
+   errx(1, "Invalid directory");
 
if (!skip_chroot && (pw = getpwnam(LDAPD_USER)) == NULL)
err(1, "%s", LDAPD_USER);

Re: ldapd: add -r option to specify datadir path

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

Gleydson Soares  writes:

>> 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.
> 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 -   1.17
> +++ ldapd.c   2 Feb 2016 10:10:43 -
> @@ -17,6 +17,7 @@
>   */
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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"?

>  
>   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, ) == -1) || !S_ISDIR(s.st_mode))
> + errx(1, "Invalid directory");

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

>  
>   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 -   1.8
+++ ldapctl.c   2 Feb 2016 12:54:12 -
@@ -22,6 +22,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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, ) == -1)
+   err(1, "%s", datadir);
+   if (!S_ISDIR(sb.st_mode))
+   errx(1, "%s is not a directory", datadir);
 
log_verbose(verbose);
 


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ldapd: add -r option to specify datadir path

[Landry Breuil]

On Tue, Feb 02, 2016 at 01:58:18PM +0100, Jérémie Courrèges-Anglas wrote:
> Gleydson Soares  writes:
> 
> >> 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.
> > 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 -   1.17
> > +++ ldapd.c 2 Feb 2016 10:10:43 -
> > @@ -17,6 +17,7 @@
> >   */
> >  
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -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"?
> 
> >  
> > 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, ) == -1) || !S_ISDIR(s.st_mode))
> > +   errx(1, "Invalid directory");
> 
> With such a diagnostic you don't get much detail about the exact
> problem.
> 
> >  
> > 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

Re: ldapd: add -r option to specify datadir path

[Gleydson Soares]

Hi Landry,

On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
> 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.
Index: ldapd.8
===
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.8,v
retrieving revision 1.12
diff -u -p -r1.12 ldapd.8
--- ldapd.8 11 Aug 2014 08:21:55 -  1.12
+++ ldapd.8 1 Feb 2016 18:51:17 -
@@ -57,6 +57,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 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.15
diff -u -p -r1.15 ldapd.c
--- ldapd.c 24 Dec 2015 17:47:57 -  1.15
+++ ldapd.c 1 Feb 2016 18:51:17 -
@@ -48,6 +48,7 @@ static voidldapd_log_verbose(struct im
 
 struct ldapd_stats  stats;
 pid_t   ldape_pid;
+char *  datadir;
 
 void
 usage(void)
@@ -55,7 +56,7 @@ usage(void)
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);
 }
 
@@ -115,9 +116,10 @@ main(int argc, char *argv[])
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;
@@ -137,6 +139,9 @@ main(int argc, char *argv[])
case 'n':
configtest = 1;
break;
+   case 'r':
+   datadir = optarg;
+   break;
case 's':
csockpath = optarg;
break;
@@ -174,6 +179,9 @@ main(int argc, char *argv[])
if (!skip_chroot && (pw = getpwnam(LDAPD_USER)) == NULL)
err(1, "%s", LDAPD_USER);
 
+   if (datadir && chdir(datadir))
+   err(1, "chdir");
+
if (!debug) {
if (daemon(1, 0) == -1)
err(1, "failed to daemonize");
@@ -343,7 +351,7 @@ ldapd_open_request(struct imsgev *iev, s
/* 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 -p -r1.14 namespace.c
--- namespace.c 24 Dec 2015 17:47:57 -  1.14
+++ namespace.c 1 Feb 2016 18:51:17 -
@@ -38,6 +38,7 @@ static voidnamespace_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)
@@ -115,7 +116,7 @@ namespace_open(struct namespace *ns)
if (ns->sync == 0)
db_flags |= BT_NOSYNC;
 
-   if (asprintf(>data_path, "%s/%s_data.db", DATADIR, ns->suffix) < 0)
+   if (asprintf(>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 @@ namespace_open(struct namespace *ns)
 
btree_set_cache_size(ns->data_db, ns->cache_size);
 
-   if (asprintf(>indx_path, 

Re: ldapd: add -r option to specify datadir path

[Landry Breuil]

On Mon, Feb 01, 2016 at 07:37:34PM +0100, Jérémie Courrèges-Anglas wrote:
> j...@wxcvbn.org (Jérémie Courrèges-Anglas) writes:
> 
> > Sebastien Marie  writes:
> >
> >> On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
> >>> 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 !
> >>> 
> >>
> >> the diff looks good and it would permit to make regress tests, so OK 
> >> semarie@ :)
> >
> > I have smallish tweaks to propose but nothing that prevents this diff to
> > go in as is.  ok jca@
> 
> 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

Re: ldapd: add -r option to specify datadir path

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

Gleydson Soares  writes:

> Hi Landry,
>
> On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
>> 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");

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

[...]

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ldapd: add -r option to specify datadir path

[Gleydson Soares]

On Mon, Feb 1, 2016 at 5:13 PM, Jérémie Courrèges-Anglas  
wrote:
> Gleydson Soares  writes:
>
>> Hi Landry,
>>
>> On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
>>> 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");
>
> Hum, while a check would be nicer, I prefer when daemons stick to /.

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

Re: ldapd: add -r option to specify datadir path

[Sebastien Marie]

On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
> 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 !
> 

the diff looks good and it would permit to make regress tests, so OK semarie@ :)

> 
> 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 -  1.12
> +++ ldapd.8   23 Jan 2016 10:11:48 -
> @@ -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 -  1.16
> +++ ldapd.c   23 Jan 2016 10:11:48 -
> @@ -50,6 +50,7 @@
>  
>  struct ldapd_statsstats;
>  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 -  1.14
> +++ namespace.c   23 Jan 2016 10:11:48 -
> @@ -38,6 +38,7 @@
>  static intnamespace_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(>data_path, "%s/%s_data.db", DATADIR, ns->suffix) < 0)
> + if (asprintf(>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(>indx_path, "%s/%s_indx.db", DATADIR, ns->suffix) < 0)
> + if (asprintf(>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

Re: ldapd: add -r option to specify datadir path

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

j...@wxcvbn.org (Jérémie Courrèges-Anglas) writes:

> Sebastien Marie  writes:
>
>> On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
>>> 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 !
>>> 
>>
>> the diff looks good and it would permit to make regress tests, so OK 
>> semarie@ :)
>
> I have smallish tweaks to propose but nothing that prevents this diff to
> go in as is.  ok jca@

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 -  1.4
+++ ldapctl.8   1 Feb 2016 17:57:55 -
@@ -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 -   1.7
+++ ldapctl.c   1 Feb 2016 17:57:55 -
@@ -56,10 +56,10 @@ void show_stats(struct imsg *imsg);
 voidshow_dbstats(const char *prefix, struct btree_stat *st);
 voidshow_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(, "%s/%s_data.db", DATADIR, ns->suffix) < 0)
+   if (asprintf(, "%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(, "%s/%s_indx.db", DATADIR, ns->suffix) < 0)
+   if (asprintf(, "%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, >namespaces, next)
-   if (SLIST_EMPTY(>referrals) && compact_namespace(ns) != 0)
+   TAILQ_FOREACH(ns, >namespaces, next) {
+   if (SLIST_EMPTY(>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(, "%s/%s_indx.db", DATADIR, ns->suffix) < 0)
+   if (asprintf(, "%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, 

Re: ldapd: add -r option to specify datadir path

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

Sebastien Marie  writes:

> On Sun, Jan 31, 2016 at 09:39:52AM +0100, Landry Breuil wrote:
>> 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 !
>> 
>
> the diff looks good and it would permit to make regress tests, so OK semarie@ 
> :)

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

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE