[SSSD] [PATCH] Read and validate configuration before daemonizing

Simo Sorce ssorce at redhat.com
Thu Sep 10 15:39:34 UTC 2009


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.
 
>  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.


> -    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.

The rest looks good.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list