[Freeipa-devel] [PATCHES][SSSD] Convert to using INI configuration file for the SSSD

[Dmitri Pal]

Simo Sorce wrote:
> On Sat, 2009-04-11 at 10:26 -0400, Stephen Gallagher wrote:
>   
>> See patch commit comments.
>>
>> Please note, review of these patches are a high priority, as
>> successful
>> completion of this task must be achieved no later than 11:00 EDT on
>> Monday in order to be included in the F11 Release Candidate and the
>> SSSD
>> Test Day.
>>     
>
> 1st patch seem ok.
>
> 3rd patch is not ok, all configuration should be performed in the setup
> step, and building only in the build step
> running autoreconf and configure in the build step is not correct
>
>
> On the 2nd I have a few comments.
>
> inotify:
> 1) we cannot simply do an if/or with inotify just because we found it in
> configure.
> Support for inotify depends on the filesystem used. So even if the
> kernel exposes the syscall we still don't know if inotify is going to be
> really available or not.
> If we fail to set up an inotify watch we should fallback to polling.
>
> Please use a function like try_inotify(...) and ifedf it to contain real
> inotify code if if we have headers or to immediately return if not like:
>
> #ifdef HAVE_INOTIFY
> int try_inotify(...) {
> ..
> }
> #else
> int try_inotify(...) {
> 	return EINVAL;
> }
> #endif
>
> and in the core code always call try_inotify() first and if EINVAL is
> returned fallback to polling.
>
> 2) inotify can return multiple events at the same time so you should
> have a loop to read them
>
> 3) you use read() without retrying in case you read less than the
> requested bytes. While a read() is unlikely to fail in this case, but
> you never know if an interrupt will interrupt the call. So you should
> really check if you need to reread.
>
> 4) You should use ioctl() to make the inotify descriptor O_NONBLOCK,
> otherwise there is a risk of blocking on the call.
>
> 5) Are we sure we want to put the file monitoring functions in confdb ?
> It seem to me monitor own code is the right place.
>
>
> files:
> 1) *never* install sssd.conf, it would overwrite existing legit ones,
> let the rpm do it. (this is definitely a blocking issue for the patch)
>
> 2) please put the example sssd.conf under examples and at the same time
> remove the ldif examples we have there as they are not necessary
> anymore. (besides, the example config file, as is is not ok, you can't
> use /lib64 on a 32 bit machine, and we don't want to use the LOCAL
> compat mode by default upstream, it's only a migration mode)
>
>
> server_setup():
> I think we should confine the config file manipulation within monitor,
> so I would prefer not to pass the conf file to the general server_setup.
>
> Let server_setup create the basic empty ldb if no ldb is found, but also
> let the monitor do all the file reading and initialization. We do not
> want to expose to all process functions that can and should be performed
> only by monitor.
>
> So move confdb_init_db() out of confdb_init() and call it from monitor
> as the first thing.
>
> (This should also reduce the number of files touched by the patch)
>
> If this is too much work for now, we can move it later, although it
> would be better to have it correct now.
>
>
>
> This is more or less all I can see that would need fixing.
> Simo.
>
>   
In follow up to Simo's comment.
Patch 2)
Function confdb_create_ldif you should free the list of sections and 
attributes in case of error. You currently do not do this.
Function confdb_init_db you should destroy error_list of parse errors 
after you printed errors using destroy_collection() function.
Function confdb_init_db you should destroy sssd_config collection after 
you used it. I do not see it being destroyed anywhere.

I know that there is a lack of time but better commenting and 
tracing/debugging capabilities in the code would really be helpful to 
understand what the  code is doing.
Also an observation. John invested several weeks into inotify kind of 
complex file monitoring.
Moving forward we should create a library out of it and put it into 
common so that you can use his work in the other parts of the sssd.
I also think that the config reload code should probably be its separate 
API based on John's code so that it can be easily used for all daemons  
written in the scope of the project whether they are a part of the sssd, 
audit or  policy client.

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/