New patch attached, comments inline.
On 09/10/2009 11:39 AM, Simo Sorce wrote:
> On Thu, 2009-09-10 at 11:11 -0400, Stephen Gallagher wrote:
>> Attached is a new approach to reading the configuration and exiting
>> with
>> an error code if the configuration is invalid. There should be no
>> situation where this will affect signal/process setup anymore.
>>
>> Patch 0001: Simple cleanup. We're now using a private event context
>> for
>> the confdb and ignoring the event_ctx argument to confdb_init, so I'm
>> removing it to eliminate confusion.
>
> Ack this one.
>
>> Patch 0002: Create the confdb and read in the configuration options
>> before daemonizing so that failures can be reported immediately.
>
> Patche is going in the right direction.
> Comments inline.
>
>
>
>> +static errno_t load_configuration(const char *config_file,
>> + struct mt_ctx **monitor)
>> +{
>> + errno_t ret;
>> + struct mt_ctx *ctx;
>> + char *cdb_file = NULL;
>> +
>> + ctx = talloc_zero(NULL, struct mt_ctx);
>
> Please pass in a memory context to this function, don't use NULL.
>
I added a tmp_ctx to the main() function that will act as the parent of
this (and the config_file string) until it's stolen.
>> int monitor_process_init(TALLOC_CTX *mem_ctx,
>> - struct tevent_context *event_ctx,
>> - struct confdb_ctx *cdb,
>> + struct mt_ctx *ctx,
>> const char *config_file)
>> {
>
> mem_ctx is not used anymore in this function as far as I can see,
> please remove it.
It is still used for sysdb_init, so I left it there.
>
>
>> - struct mt_ctx *ctx;
>> @@ -2332,6 +2343,7 @@ int main(int argc, const char *argv[])
>> char *config_file = NULL;
>> int flags = 0;
>> struct main_context *main_ctx;
>> + struct mt_ctx *monitor;
>> int ret;
>>
>> struct poptOption long_options[] = {
>> @@ -2379,16 +2391,18 @@ int main(int argc, const char *argv[])
>> flags |= FLAGS_PID_FILE;
>>
>> /* Parse config file, fail if cannot be done */
>> - ret = read_config_file(config_file);
>> + ret = load_configuration(config_file, &monitor);
>> if (ret != EOK) return 4;
>>
>> /* set up things like debug , signals, daemonization, etc... */
>> ret = server_setup("sssd", flags, MONITOR_CONF_ENTRY, &main_ctx);
>> if (ret != EOK) return 2;
>>
>> + monitor->ev = main_ctx->event_ctx;
>> + talloc_steal(main_ctx, monitor);
>> +
>
> It would be nice to refactor the code to avoid this steal here, although
> that may prove to be a bit complicated.
> One way could be to allocate main_ctx outside of server_setup() before
> load_configuration() is called, and make it not be a child of the event
> context.
> Although I wouldn't consider a decision not to to this as reason to
> nack.
It's a fairly complicated change to do that. I think it deserves its own
patch somewhere down the road.
>
> The rest looks good.
>
> Simo.
>
--
Stephen Gallagher
RHCE 804006346421761
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
From d5170bc64cba573d2a3b504c519f439cf763a48d Mon Sep 17 00:00:00 2001 From: Stephen Gallagher <[email protected]> Date: Thu, 10 Sep 2009 09:26:28 -0400 Subject: [PATCH 1/2] Remove unused event context argument from confdb_init Because the confdb always operates synchronously, it maintains its own private event context internally. The event context argument passed to it is never used, so we'll remove it to avoid confusion. --- server/confdb/confdb.c | 1 - server/confdb/confdb.h | 1 - server/monitor/monitor.c | 2 +- server/tests/sysdb-tests.c | 2 +- server/tools/tools_util.c | 2 +- server/util/server.c | 4 ++-- 6 files changed, 5 insertions(+), 7 deletions(-) diff --git a/server/confdb/confdb.c b/server/confdb/confdb.c index 27daee8..5d7bb65 100644 --- a/server/confdb/confdb.c +++ b/server/confdb/confdb.c @@ -627,7 +627,6 @@ done: } int confdb_init(TALLOC_CTX *mem_ctx, - struct tevent_context *ev, struct confdb_ctx **cdb_ctx, char *confdb_location) { diff --git a/server/confdb/confdb.h b/server/confdb/confdb.h index d325ac8..d49040f 100644 --- a/server/confdb/confdb.h +++ b/server/confdb/confdb.h @@ -86,7 +86,6 @@ int confdb_get_string_as_list(struct confdb_ctx *cdb, TALLOC_CTX *ctx, char ***result); int confdb_init(TALLOC_CTX *mem_ctx, - struct tevent_context *ev, struct confdb_ctx **cdb_ctx, char *confdb_location); diff --git a/server/monitor/monitor.c b/server/monitor/monitor.c index 86b2364..cd4e3ce 100644 --- a/server/monitor/monitor.c +++ b/server/monitor/monitor.c @@ -1811,7 +1811,7 @@ int monitor_process_init(TALLOC_CTX *mem_ctx, } unlink(cdb_file); - ret = confdb_init(ctx, ctx->ev, &ctx->cdb, cdb_file); + ret = confdb_init(ctx, &ctx->cdb, cdb_file); if (ret != EOK) { DEBUG(0,("The confdb initialization failed\n")); talloc_free(ctx); diff --git a/server/tests/sysdb-tests.c b/server/tests/sysdb-tests.c index 38bfcb1..5250b08 100644 --- a/server/tests/sysdb-tests.c +++ b/server/tests/sysdb-tests.c @@ -81,7 +81,7 @@ static int setup_sysdb_tests(struct sysdb_test_ctx **ctx) DEBUG(3, ("CONFDB: %s\n", conf_db)); /* Connect to the conf db */ - ret = confdb_init(test_ctx, test_ctx->ev, &test_ctx->confdb, conf_db); + ret = confdb_init(test_ctx, &test_ctx->confdb, conf_db); if (ret != EOK) { fail("Could not initialize connection to the confdb"); talloc_free(test_ctx); diff --git a/server/tools/tools_util.c b/server/tools/tools_util.c index c23899c..15ba16f 100644 --- a/server/tools/tools_util.c +++ b/server/tools/tools_util.c @@ -76,7 +76,7 @@ int setup_db(struct tools_ctx **tools_ctx) } /* Connect to the conf db */ - ret = confdb_init(ctx, ctx->ev, &ctx->confdb, confdb_path); + ret = confdb_init(ctx, &ctx->confdb, confdb_path); if (ret != EOK) { DEBUG(1, ("Could not initialize connection to the confdb\n")); talloc_free(ctx); diff --git a/server/util/server.c b/server/util/server.c index d20a823..0760e60 100644 --- a/server/util/server.c +++ b/server/util/server.c @@ -47,7 +47,7 @@ static void close_low_fds(bool stderr_too) int i; close(0); - close(1); + close(1); if (stderr_too) close(2); @@ -354,7 +354,7 @@ int server_setup(const char *name, int flags, } DEBUG(3, ("CONFDB: %s\n", conf_db)); - ret = confdb_init(ctx, event_ctx, &ctx->confdb_ctx, conf_db); + ret = confdb_init(ctx, &ctx->confdb_ctx, conf_db); if (ret != EOK) { DEBUG(0,("The confdb initialization failed\n")); return ret; -- 1.6.2.5
From 09cb058d41feea77130ebfb55f2a787e00512544 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher <[email protected]> Date: Thu, 10 Sep 2009 10:22:50 -0400 Subject: [PATCH 2/2] Read the configuration parsing before daemonization We will now parse the config file and validate the confdb contents before processing the rest of the monitor startup. This will allow us to return an appropriate error code to the shell if the configuration is invalid. --- server/monitor/monitor.c | 171 ++++++++++++++++++++++++++-------------------- server/monitor/monitor.h | 5 +- 2 files changed, 99 insertions(+), 77 deletions(-) diff --git a/server/monitor/monitor.c b/server/monitor/monitor.c index cd4e3ce..0310fef 100644 --- a/server/monitor/monitor.c +++ b/server/monitor/monitor.c @@ -1385,6 +1385,86 @@ int read_config_file(const char *config_file) return ret; } +static errno_t load_configuration(TALLOC_CTX *mem_ctx, + const char *config_file, + struct mt_ctx **monitor) +{ + errno_t ret; + struct mt_ctx *ctx; + char *cdb_file = NULL; + + ctx = talloc_zero(mem_ctx, struct mt_ctx); + if(!ctx) { + return ENOMEM; + } + + cdb_file = talloc_asprintf(ctx, "%s/%s", DB_PATH, CONFDB_FILE); + if (cdb_file == NULL) { + DEBUG(0,("Out of memory, aborting!\n")); + ret = ENOMEM; + goto done; + } + + ret = confdb_init(ctx, &ctx->cdb, cdb_file); + if (ret != EOK) { + DEBUG(0,("The confdb initialization failed\n")); + goto done; + } + talloc_free(cdb_file); + + /* Initialize the CDB from the configuration file */ + ret = confdb_test(ctx->cdb); + if (ret == ENOENT) { + /* First-time setup */ + + /* Purge any existing confdb in case an old + * misconfiguration gets in the way + */ + talloc_zfree(ctx->cdb); + unlink(cdb_file); + + ret = confdb_init(ctx, &ctx->cdb, cdb_file); + if (ret != EOK) { + DEBUG(0,("The confdb initialization failed\n")); + goto done; + } + + /* Load special entries */ + ret = confdb_create_base(ctx->cdb); + if (ret != EOK) { + DEBUG(0, ("Unable to load special entries into confdb\n")); + goto done; + } + } else if (ret != EOK) { + DEBUG(0, ("Fatal error initializing confdb\n")); + goto done; + } + + ret = confdb_init_db(config_file, ctx->cdb); + if (ret != EOK) { + DEBUG(0, ("ConfDB initialization has failed [%s]\n", + strerror(ret))); + goto done; + } + + /* Validate the configuration in the database */ + /* Read in the monitor's configuration */ + ret = get_monitor_config(ctx); + if (ret != EOK) { + goto done; + } + + *monitor = ctx; + + ret = EOK; + +done: + if (ret != EOK) { + talloc_free(ctx); + } + return ret; +} + #ifdef HAVE_SYS_INOTIFY_H static void process_config_file(struct tevent_context *ev, struct tevent_timer *te, @@ -1775,76 +1855,14 @@ static int monitor_config_file(TALLOC_CTX *mem_ctx, } int monitor_process_init(TALLOC_CTX *mem_ctx, - struct tevent_context *event_ctx, - struct confdb_ctx *cdb, + struct mt_ctx *ctx, const char *config_file) { - struct mt_ctx *ctx; struct sysdb_ctx_list *db_list; struct tevent_signal *tes; int ret, i; - char *cdb_file; struct sss_domain_info *dom; - ctx = talloc_zero(mem_ctx, struct mt_ctx); - if (!ctx) { - DEBUG(0, ("fatal error initializing monitor!\n")); - return ENOMEM; - } - ctx->ev = event_ctx; - ctx->cdb = cdb; - - /* Initialize the CDB from the configuration file */ - ret = confdb_test(ctx->cdb); - if (ret == ENOENT) { - /* First-time setup */ - - /* Purge any existing confdb in case an old - * misconfiguration gets in the way - */ - talloc_free(ctx->cdb); - cdb_file = talloc_asprintf(ctx, "%s/%s", DB_PATH, CONFDB_FILE); - if (cdb_file == NULL) { - DEBUG(0,("Out of memory, aborting!\n")); - talloc_free(ctx); - return ENOMEM; - } - - unlink(cdb_file); - ret = confdb_init(ctx, &ctx->cdb, cdb_file); - if (ret != EOK) { - DEBUG(0,("The confdb initialization failed\n")); - talloc_free(ctx); - return ret; - } - - /* Load special entries */ - ret = confdb_create_base(ctx->cdb); - if (ret != EOK) { - talloc_free(ctx); - return ret; - } - } else if (ret != EOK) { - DEBUG(0, ("Fatal error initializing confdb\n")); - talloc_free(ctx); - return ret; - } - - ret = confdb_init_db(config_file, ctx->cdb); - if (ret != EOK) { - DEBUG(0, ("ConfDB initialization has failed [%s]\n", - strerror(ret))); - talloc_free(ctx); - return ret; - } - - /* Read in the monitor's configuration */ - ret = get_monitor_config(ctx); - if (ret != EOK) { - talloc_free(ctx); - return ret; - } - /* Watch for changes to the confdb config file */ ret = monitor_config_file(ctx, ctx, config_file, monitor_signal_reconf); if (ret != EOK) { @@ -1856,7 +1874,6 @@ int monitor_process_init(TALLOC_CTX *mem_ctx, ret = monitor_config_file(ctx, ctx, RESOLV_CONF_PATH, monitor_update_resolv); if (ret != EOK) { - talloc_free(ctx); return ret; } @@ -1866,7 +1883,6 @@ int monitor_process_init(TALLOC_CTX *mem_ctx, */ ret = sysdb_init(mem_ctx, ctx->ev, ctx->cdb, NULL, true, &db_list); if (ret != EOK) { - talloc_free(ctx); return ret; } talloc_free(db_list); @@ -1876,7 +1892,6 @@ int monitor_process_init(TALLOC_CTX *mem_ctx, * SSSD processes */ ret = monitor_dbus_init(ctx); if (ret != EOK) { - talloc_free(ctx); return ret; } @@ -1897,7 +1912,6 @@ int monitor_process_init(TALLOC_CTX *mem_ctx, tes = tevent_add_signal(ctx->ev, ctx, SIGHUP, 0, monitor_hup, ctx); if (tes == NULL) { - talloc_free(ctx); return EIO; } @@ -1905,7 +1919,6 @@ int monitor_process_init(TALLOC_CTX *mem_ctx, tes = tevent_add_signal(ctx->ev, ctx, SIGINT, 0, monitor_quit, ctx); if (tes == NULL) { - talloc_free(ctx); return EIO; } @@ -1913,7 +1926,6 @@ int monitor_process_init(TALLOC_CTX *mem_ctx, tes = tevent_add_signal(ctx->ev, ctx, SIGTERM, 0, monitor_quit, ctx); if (tes == NULL) { - talloc_free(ctx); return EIO; } @@ -2332,6 +2344,8 @@ int main(int argc, const char *argv[]) char *config_file = NULL; int flags = 0; struct main_context *main_ctx; + TALLOC_CTX *tmp_ctx; + struct mt_ctx *monitor; int ret; struct poptOption long_options[] = { @@ -2365,13 +2379,18 @@ int main(int argc, const char *argv[]) poptFreeContext(pc); + tmp_ctx = talloc_new(NULL); + if (!tmp_ctx) { + return 7; + } + if (opt_daemon) flags |= FLAGS_DAEMON; if (opt_interactive) flags |= FLAGS_INTERACTIVE; if (opt_config_file) - config_file = talloc_strdup(NULL, opt_config_file); + config_file = talloc_strdup(tmp_ctx, opt_config_file); else - config_file = talloc_strdup(NULL, CONFDB_DEFAULT_CONFIG_FILE); + config_file = talloc_strdup(tmp_ctx, CONFDB_DEFAULT_CONFIG_FILE); if(!config_file) return 6; @@ -2379,19 +2398,21 @@ int main(int argc, const char *argv[]) flags |= FLAGS_PID_FILE; /* Parse config file, fail if cannot be done */ - ret = read_config_file(config_file); + ret = load_configuration(tmp_ctx, config_file, &monitor); if (ret != EOK) return 4; /* set up things like debug , signals, daemonization, etc... */ ret = server_setup("sssd", flags, MONITOR_CONF_ENTRY, &main_ctx); if (ret != EOK) return 2; + monitor->ev = main_ctx->event_ctx; + talloc_steal(main_ctx, monitor); + ret = monitor_process_init(main_ctx, - main_ctx->event_ctx, - main_ctx->confdb_ctx, + monitor, config_file); if (ret != EOK) return 3; - talloc_free(config_file); + talloc_free(tmp_ctx); /* loop on main */ server_loop(main_ctx); diff --git a/server/monitor/monitor.h b/server/monitor/monitor.h index 0127bbf..6ea55ea 100644 --- a/server/monitor/monitor.h +++ b/server/monitor/monitor.h @@ -28,9 +28,10 @@ typedef int (*monitor_reconf_fn) (struct config_file_ctx *file_ctx, const char *filename); +struct mt_ctx; + int monitor_process_init(TALLOC_CTX *mem_ctx, - struct tevent_context *event_ctx, - struct confdb_ctx *cdb, + struct mt_ctx *ctx, const char *config_file); #endif /* _MONITOR_H */ -- 1.6.2.5
signature.asc
Description: OpenPGP digital signature
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
