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

[Stephen Gallagher]

Dmitri Pal wrote:
> 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.
> 

I have made all of the changes recommended by Simo and Dmitri. Please
re-review when you can. The new patches are attached.

-- 
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-Build-system-improvements-for-common-tools.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090412/426442f1/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-Allow-configuration-of-the-SSSD-through-etc-sssd-ss.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090412/426442f1/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0003-Update-RPM-build-for-configuration-changes.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090412/426442f1/attachment-0002.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 259 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090412/426442f1/attachment.sig>