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

[Dmitri Pal]

Dmitri Pal wrote:
> 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.
>
And one other thing I forgot to mention. The collection is not intended 
to store large sets of data as you said in the package description. The 
collection is more oriented on the hierarchical complex sets of data but 
not big in size (dozens of items may be a hundred, but not thousands). 
It is good for collecting and iterating (serializing) but not optimal 
for search.
With thousands items the hash table implementation might be a more 
preferable approach.
In future however the collection can be improved to take advantage of 
hash table internally but currently it is not the case.

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


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