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

Stephen Gallagher sgallagh at redhat.com
Thu Sep 10 20:18:07 UTC 2009


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/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Remove-unused-event-context-argument-from-confdb_ini.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090910/7e5b5d4b/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-Read-the-configuration-parsing-before-daemonization.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090910/7e5b5d4b/attachment-0001.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 260 bytes
Desc: OpenPGP digital signature
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090910/7e5b5d4b/attachment.sig>


More information about the sssd-devel mailing list