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

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York