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

[Stephen Gallagher]

Stephen Gallagher wrote:
> 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.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

One more set of patches based on IRC discussion. Only patch 0002 has
changed from the previous mailing.



-- 
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/20090413/dc1d98c1/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/20090413/dc1d98c1/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/20090413/dc1d98c1/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/20090413/dc1d98c1/attachment.sig>