This reads fine to me and fixes the regress test location.
Does anyone else want to chime in?
martijn@
On 3/4/20 9:11 PM, Robert Klein wrote:
> Hi,
>
> On Tue, 3 Mar 2020 20:45:17 +0100
> Martijn van Duren <openbsd+t...@list.imperialat.at> wrote:
>
>> I don't agree with this diff, even though you're right with it being
>> broken. Right now the regress test uses it/tries to use it and even
>> though someone trying to run regress on an actual production machine
>> somewhat deserves to get shot in the foot, I don't think we should
>> make that our goal.
>>
>> Also, I can see this being a helpful feature for people doing some
>> development work (e.g. testing with delegations etc without setting
>> up a vm-environment).
>>
>> Could you please look into fixing the actual issue?
>
> this patch fixes only the two issues. I de-const'd "char * datadir" in
> ldapd to get rid of the new warning.
>
> Best regards
> Robert
>
>
> diff 1813335e849f285a868ea3d474b4704812b1843e /usr/src
> blob - c8564c5543f518a720e049c559556f87edda6b8a
> file + usr.sbin/ldapctl/ldapctl.c
> --- usr.sbin/ldapctl/ldapctl.c
> +++ usr.sbin/ldapctl/ldapctl.c
> @@ -150,7 +150,7 @@ index_namespace(struct namespace *ns, const char *data
>
> log_info("indexing namespace %s", ns->suffix);
>
> - if (asprintf(&path, "%s/%s_data.db", DATADIR, ns->suffix) == -1)
> + if (asprintf(&path, "%s/%s_data.db", datadir, ns->suffix) == -1)
> return -1;
> data_db = btree_open(path, BT_NOSYNC | BT_REVERSEKEY, 0644);
> free(path);
> blob - 797a36f89f33233c2d9655a307ec5e727b9fbaa1
> file + usr.sbin/ldapd/ldapd.c
> --- usr.sbin/ldapd/ldapd.c
> +++ usr.sbin/ldapd/ldapd.c
> @@ -55,7 +55,7 @@ static pid_t start_child(enum ldapd_process, char
> *,
>
> struct ldapd_stats stats;
> pid_t ldape_pid;
> -const char *datadir = DATADIR;
> +char *datadir = DATADIR;
>
> void
> usage(void)
> @@ -415,7 +415,7 @@ static pid_t
> start_child(enum ldapd_process p, char *argv0, int fd, int debug,
> int verbose, char *csockpath, char *conffile)
> {
> - char *argv[9];
> + char *argv[11];
> int argc = 0;
> pid_t pid;
>
> @@ -459,7 +459,11 @@ start_child(enum ldapd_process p, char *argv0, int fd,
> argv[argc++] = "-f";
> argv[argc++] = conffile;
> }
> -
> + if (datadir) {
> + argv[argc++] = "-r";
> + argv[argc++] = datadir;
> + }
> +
> argv[argc++] = NULL;
>
> execvp(argv0, argv);
> blob - 2259f57bde3c2918a8b200747c194e5768ae40f1
> file + usr.sbin/ldapd/namespace.c
> --- usr.sbin/ldapd/namespace.c
> +++ usr.sbin/ldapd/namespace.c
> @@ -29,7 +29,7 @@
> #include "ldapd.h"
> #include "log.h"
>
> -extern const char *datadir;
> +extern char *datadir;
>
> /* Maximum number of requests to queue per namespace during compaction.
> * After this many requests, we return LDAP_BUSY.
>
>>
>> martijn@
>>
>> On 3/2/20 8:00 PM, Robert Klein wrote:
>>> Hi,
>>>
>>> in ldapd and ldapctl the "-r directory" command line argument does
>>> not work:
>>>
>>> ldapd fork/execs itself but the directory command line argument is
>>> not given to the execvp call which then uses the default data
>>> directory.
>>>
>>> ldapctl has a thinko/typo which causes the data_db to be always
>>> opened in the default directory for indexing.
>>>
>>> The patch below removes the command line argument and corresponding
>>> global variable and instead uses a configuration file directive
>>> "datadir". Incidentally removes the global 'datadir' variable used
>>> before.
>>>
>>> Best regards
>>> Robert
>>>
>>>
>>> -----------------------------------------------
>>> commit 6a71c90c19b5dc5850305c15696af3f14d26c168 (master)
>>> from: Robert Klein <rokl...@roklein.de>
>>> date: Mon Mar 2 18:50:12 2020 UTC
>>>
>>> fix ldapd/ldapctl data directory handling
>>>
>>> -r directive didn't work as intended in both
>>> ldapd and ldapctl.
>>>
>>> this patch replaces command line argument '-r directory'
>>> with a configuration file directive 'datadir'.
>>>
>>> diff f62b80b397c780e8273b5cc67d544b951c4c9d1f
>>> 35bb29194b2067dab925ec4566dfb82f9bf34d65 blob -
>>> 48cff01b435bca0deebf28f55e6f71675c5752f2 blob +
>>> 231af2ddbbee2da446d2f03c9c48e679e216e993 ---
>>> usr.sbin/ldapctl/ldapctl.8 +++ usr.sbin/ldapctl/ldapctl.8
>>> @@ -24,7 +24,6 @@
>>> .Nm ldapctl
>>> .Op Fl v
>>> .Op Fl f Ar file
>>> -.Op Fl r Ar directory
>>> .Op Fl s Ar socket
>>> .Ar command
>>> .Op Ar argument ...
>>> @@ -42,11 +41,6 @@ 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
>>> blob - c8564c5543f518a720e049c559556f87edda6b8a
>>> blob + 6830a603d153794390faafbc9aadc8ef0bd985c0
>>> --- usr.sbin/ldapctl/ldapctl.c
>>> +++ usr.sbin/ldapctl/ldapctl.c
>>> @@ -58,10 +58,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, const
>>> char *datadir); -int compact_namespaces(const char
>>> *datadir); -int index_namespace(struct namespace
>>> *ns, const char *datadir); -int
>>> index_namespaces(const char *datadir); +int
>>> compact_namespace(struct namespace *ns); +int
>>> compact_namespaces(void); +int
>>> index_namespace(struct namespace *ns); +int
>>> index_namespaces(void); int
>>> ssl_load_certfile(struct ldapd_config *, const char *, u_int8_t);
>>> __dead void
>>> @@ -70,7 +70,7 @@ usage(void)
>>> extern char *__progname;
>>>
>>> fprintf(stderr,
>>> - "usage: %s [-v] [-f file] [-r directory] [-s socket] "
>>> + "usage: %s [-v] [-f file] [-s socket] "
>>> "command [argument ...]\n",
>>> __progname);
>>> exit(1);
>>> @@ -97,11 +97,11 @@ compact_db(const char *path)
>>> }
>>>
>>> int
>>> -compact_namespace(struct namespace *ns, const char *datadir)
>>> +compact_namespace(struct namespace *ns)
>>> {
>>> char *path;
>>>
>>> - if (asprintf(&path, "%s/%s_data.db", datadir, ns->suffix)
>>> == -1)
>>> + if (asprintf(&path, "%s/%s_data.db", conf->datadir,
>>> ns->suffix) == -1) return -1;
>>> if (compact_db(path) != 0) {
>>> log_warn("%s", path);
>>> @@ -110,7 +110,7 @@ compact_namespace(struct namespace *ns, const
>>> char *da }
>>> free(path);
>>>
>>> - if (asprintf(&path, "%s/%s_indx.db", datadir, ns->suffix)
>>> == -1)
>>> + if (asprintf(&path, "%s/%s_indx.db", conf->datadir,
>>> ns->suffix) == -1) return -1;
>>> if (compact_db(path) != 0) {
>>> log_warn("%s", path);
>>> @@ -123,14 +123,14 @@ compact_namespace(struct namespace *ns, const
>>> char *da }
>>>
>>> int
>>> -compact_namespaces(const char *datadir)
>>> +compact_namespaces()
>>> {
>>> struct namespace *ns;
>>>
>>> TAILQ_FOREACH(ns, &conf->namespaces, next) {
>>> if (SLIST_EMPTY(&ns->referrals))
>>> continue;
>>> - if (compact_namespace(ns, datadir) != 0)
>>> + if (compact_namespace(ns) != 0)
>>> return -1;
>>> }
>>>
>>> @@ -138,7 +138,7 @@ compact_namespaces(const char *datadir)
>>> }
>>>
>>> int
>>> -index_namespace(struct namespace *ns, const char *datadir)
>>> +index_namespace(struct namespace *ns)
>>> {
>>> struct btval key, val;
>>> struct btree *data_db, *indx_db;
>>> @@ -150,14 +150,14 @@ index_namespace(struct namespace *ns, const
>>> char *data
>>> log_info("indexing namespace %s", ns->suffix);
>>>
>>> - if (asprintf(&path, "%s/%s_data.db", DATADIR, ns->suffix)
>>> == -1)
>>> + if (asprintf(&path, "%s/%s_data.db", conf->datadir,
>>> ns->suffix) == -1) return -1;
>>> data_db = btree_open(path, BT_NOSYNC | BT_REVERSEKEY,
>>> 0644); free(path);
>>> if (data_db == NULL)
>>> return -1;
>>>
>>> - if (asprintf(&path, "%s/%s_indx.db", datadir, ns->suffix)
>>> == -1)
>>> + if (asprintf(&path, "%s/%s_indx.db", conf->datadir,
>>> ns->suffix) == -1) return -1;
>>> indx_db = btree_open(path, BT_NOSYNC, 0644);
>>> free(path);
>>> @@ -219,14 +219,14 @@ index_namespace(struct namespace *ns, const
>>> char *data }
>>>
>>> int
>>> -index_namespaces(const char *datadir)
>>> +index_namespaces()
>>> {
>>> struct namespace *ns;
>>>
>>> TAILQ_FOREACH(ns, &conf->namespaces, next) {
>>> if (SLIST_EMPTY(&ns->referrals))
>>> continue;
>>> - if (index_namespace(ns, datadir) != 0)
>>> + if (index_namespace(ns) != 0)
>>> return -1;
>>> }
>>>
>>> @@ -247,7 +247,6 @@ main(int argc, char *argv[])
>>> ssize_t n;
>>> int ch;
>>> enum action action = NONE;
>>> - const char *datadir = DATADIR;
>>> struct stat sb;
>>> const char *sock = LDAPD_SOCKET;
>>> char *conffile = CONFFILE;
>>> @@ -262,9 +261,6 @@ main(int argc, char *argv[])
>>> case 'f':
>>> conffile = optarg;
>>> break;
>>> - case 'r':
>>> - datadir = optarg;
>>> - break;
>>> case 's':
>>> sock = optarg;
>>> break;
>>> @@ -282,11 +278,6 @@ 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);
>>> -
>>> ldap_loginit(NULL, 1, verbose);
>>>
>>> if (strcmp(argv[0], "stats") == 0)
>>> @@ -311,13 +302,22 @@ main(int argc, char *argv[])
>>> if (parse_config(conffile) != 0)
>>> exit(2);
>>>
>>> + if (conf->datadir == NULL) {
>>> + if (asprintf(&conf->datadir, "%s",
>>> LDAPD_DATADIR) == -1)
>>> + errx(1, "malloc failed");
>>> + }
>>> + if (stat(conf->datadir, &sb) == -1)
>>> + err(1, "%s", conf->datadir);
>>> + if (!S_ISDIR(sb.st_mode))
>>> + errx(1, "%s is not a directory",
>>> conf->datadir); +
>>> if (pledge("stdio rpath wpath cpath flock", NULL)
>>> == -1) err(1, "pledge");
>>>
>>> if (action == COMPACT_DB)
>>> - return compact_namespaces(datadir);
>>> + return compact_namespaces();
>>> else
>>> - return index_namespaces(datadir);
>>> + return index_namespaces();
>>> }
>>>
>>> /* connect to ldapd control socket */
>>> blob - db5a0be5ad93c11c0dd773d342ea131328dcaa82
>>> blob + 8588bbe3dc5ffd81b91967cf809681b1998cbd69
>>> --- usr.sbin/ldapd/ldapd.8
>>> +++ usr.sbin/ldapd/ldapd.8
>>> @@ -27,7 +27,6 @@
>>> .Fl D Ar macro Ns = Ns Ar value
>>> .Oc
>>> .Op Fl f Ar file
>>> -.Op Fl r Ar directory
>>> .Op Fl s Ar file
>>> .Sh DESCRIPTION
>>> .Nm
>>> @@ -61,11 +60,6 @@ as the configuration file, instead of the default
>>> .It Fl n
>>> Configtest mode.
>>> Only check the configuration file for validity.
>>> -.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 file
>>> Specify an alternative location for the socket file.
>>> .It Fl v
>>> blob - 797a36f89f33233c2d9655a307ec5e727b9fbaa1
>>> blob + 69ae065ae99ba9ab326afd1fe0db5eeb820ea629
>>> --- usr.sbin/ldapd/ldapd.c
>>> +++ usr.sbin/ldapd/ldapd.c
>>> @@ -55,7 +55,6 @@ static pid_t start_child(enum
>>> ldapd_process, char *,
>>> struct ldapd_stats stats;
>>> pid_t ldape_pid;
>>> -const char *datadir = DATADIR;
>>>
>>> void
>>> usage(void)
>>> @@ -63,7 +62,7 @@ usage(void)
>>> extern char *__progname;
>>>
>>> fprintf(stderr, "usage: %s [-dnv] [-D macro=value] "
>>> - "[-f file] [-r directory] [-s file]\n", __progname);
>>> + "[-f file] [-s file]\n", __progname);
>>> exit(1);
>>> }
>>>
>>> @@ -151,9 +150,6 @@ main(int argc, char *argv[])
>>> case 'n':
>>> configtest = 1;
>>> break;
>>> - case 'r':
>>> - datadir = optarg;
>>> - break;
>>> case 's':
>>> csockpath = optarg;
>>> break;
>>> @@ -193,15 +189,20 @@ main(int argc, char *argv[])
>>> exit(0);
>>> }
>>>
>>> + if (conf->datadir == NULL) {
>>> + if (asprintf(&conf->datadir, "%s", LDAPD_DATADIR)
>>> == -1)
>>> + errx(1, "malloc failed");
>>> + }
>>> +
>>> log_init(debug, LOG_DAEMON);
>>>
>>> if (eflag)
>>> ldape(debug, verbose, csockpath);
>>>
>>> - if (stat(datadir, &sb) == -1)
>>> - err(1, "%s", datadir);
>>> + if (stat(conf->datadir, &sb) == -1)
>>> + err(1, "%s", conf->datadir);
>>> if (!S_ISDIR(sb.st_mode))
>>> - errx(1, "%s is not a directory", datadir);
>>> + errx(1, "%s is not a directory", conf->datadir);
>>>
>>> if (!debug) {
>>> if (daemon(1, 0) == -1)
>>> @@ -213,7 +214,7 @@ main(int argc, char *argv[])
>>> if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC |
>>> SOCK_NONBLOCK, PF_UNSPEC, pipe_parent2ldap) != 0)
>>> fatal("socketpair");
>>> -
>>> +
>>> ldape_pid = start_child(PROC_LDAP_SERVER, saved_argv0,
>>> pipe_parent2ldap[1], debug, verbose, csockpath,
>>> conffile);
>>> @@ -392,7 +393,7 @@ ldapd_open_request(struct imsgev *iev, struct
>>> imsg *im /* make sure path is null-terminated */
>>> oreq->path[PATH_MAX] = '\0';
>>>
>>> - if (strncmp(oreq->path, datadir, strlen(datadir)) != 0) {
>>> + if (strncmp(oreq->path, conf->datadir,
>>> strlen(conf->datadir)) != 0) { log_warnx("refusing to open file
>>> %s", oreq->path); fatal("ldape sent invalid open request");
>>> }
>>> @@ -459,7 +460,7 @@ start_child(enum ldapd_process p, char *argv0,
>>> int fd, argv[argc++] = "-f";
>>> argv[argc++] = conffile;
>>> }
>>> -
>>> +
>>> argv[argc++] = NULL;
>>>
>>> execvp(argv0, argv);
>>> blob - 3da0137e137e8b3bc64f6351579fcd6a9344b1f8
>>> blob + 31c894c3e39ab70f3324415ecac5efc1301e0a54
>>> --- usr.sbin/ldapd/ldapd.conf.5
>>> +++ usr.sbin/ldapd/ldapd.conf.5
>>> @@ -130,6 +130,11 @@ The URL has the following format:
>>> ldap://ldap.example.com
>>> ldaps://ldap.example.com:3890
>>> .Ed
>>> +.It datadir Ar directory
>>> +Store and read database files in
>>> +.Ar directory ,
>>> +instead of the default
>>> +.Pa /var/db/ldap .
>>> .It rootdn Ar dn
>>> Specify the distinguished name of the root user for all namespaces.
>>> The root user is always allowed to read and write entries in all
>>> blob - 3f995d184b442f094a95f089e3bb4a727eabf559
>>> blob + 2736d366bf5f92e3579f915f4ceb2a3610d27d54
>>> --- usr.sbin/ldapd/ldapd.h
>>> +++ usr.sbin/ldapd/ldapd.h
>>> @@ -41,7 +41,7 @@
>>> #define CONFFILE "/etc/ldapd.conf"
>>> #define LDAPD_USER "_ldapd"
>>> #define LDAPD_SOCKET "/var/run/ldapd.sock"
>>> -#define DATADIR "/var/db/ldap"
>>> +#define LDAPD_DATADIR "/var/db/ldap"
>>> #define LDAP_PORT 389
>>> #define LDAPS_PORT 636
>>> #define LDAPD_SESSION_TIMEOUT 30
>>> @@ -252,6 +252,7 @@ struct ldapd_config
>>> struct schema *schema;
>>> char *rootdn;
>>> char *rootpw;
>>> + char *datadir;
>>> };
>>>
>>> struct ldapd_stats
>>> blob - 2259f57bde3c2918a8b200747c194e5768ae40f1
>>> blob + 2e39abf6cc94ac6f301876df996a6229a032a7b7
>>> --- usr.sbin/ldapd/namespace.c
>>> +++ usr.sbin/ldapd/namespace.c
>>> @@ -29,8 +29,6 @@
>>> #include "ldapd.h"
>>> #include "log.h"
>>>
>>> -extern const char *datadir;
>>> -
>>> /* Maximum number of requests to queue per namespace during
>>> compaction.
>>> * After this many requests, we return LDAP_BUSY.
>>> */
>>> @@ -118,7 +116,8 @@ namespace_open(struct namespace *ns)
>>> if (ns->sync == 0)
>>> db_flags |= BT_NOSYNC;
>>>
>>> - if (asprintf(&ns->data_path, "%s/%s_data.db", datadir,
>>> ns->suffix) == -1)
>>> + if (asprintf(&ns->data_path, "%s/%s_data.db",
>>> conf->datadir,
>>> + ns->suffix) == -1)
>>> return -1;
>>> log_info("opening namespace %s", ns->suffix);
>>> ns->data_db = btree_open(ns->data_path, db_flags |
>>> BT_REVERSEKEY, 0644); @@ -127,7 +126,8 @@ namespace_open(struct
>>> namespace *ns)
>>> btree_set_cache_size(ns->data_db, ns->cache_size);
>>>
>>> - if (asprintf(&ns->indx_path, "%s/%s_indx.db", datadir,
>>> ns->suffix) == -1)
>>> + if (asprintf(&ns->indx_path, "%s/%s_indx.db",
>>> conf->datadir,
>>> + ns->suffix) == -1)
>>> return -1;
>>> ns->indx_db = btree_open(ns->indx_path, db_flags, 0644);
>>> if (ns->indx_db == NULL)
>>> blob - bad9bc630403e5280e3b0db41d8ec1247db70352
>>> blob + b72e4d89324b332498f8e5e0d1d8db822d4588f9
>>> --- usr.sbin/ldapd/parse.y
>>> +++ usr.sbin/ldapd/parse.y
>>> @@ -117,7 +117,7 @@ static struct namespace *current_ns = NULL;
>>> %}
>>>
>>> %token ERROR LISTEN ON TLS LDAPS PORT NAMESPACE ROOTDN
>>> ROOTPW INDEX -%token SECURE RELAX STRICT SCHEMA USE
>>> COMPRESSION LEVEL +%token SECURE RELAX STRICT SCHEMA USE
>>> COMPRESSION LEVEL DATADIR %token INCLUDE CERTIFICATE FSYNC
>>> CACHE_SIZE INDEX_CACHE_SIZE %token DENY ALLOW READ WRITE
>>> BIND ACCESS TO ROOT REFERRAL %token ANY CHILDREN OF
>>> ATTRIBUTE IN SUBTREE BY SELF @@ -225,6 +225,7 @@ conf_main :
>>> LISTEN ON STRING port ssl certname {
>>> normalize_dn(conf->rootdn); }
>>> | ROOTPW STRING {
>>> conf->rootpw = $2; }
>>> + | DATADIR STRING { conf->datadir =
>>> $2; } ;
>>>
>>> namespace : NAMESPACE STRING '{' '\n' {
>>> @@ -441,6 +442,7 @@ lookup(char *s)
>>> { "certificate", CERTIFICATE },
>>> { "children", CHILDREN },
>>> { "compression", COMPRESSION },
>>> + { "datadir", DATADIR },
>>> { "deny", DENY },
>>> { "fsync", FSYNC },
>>> { "in", IN },
>>>
>
