On (20/06/16 16:13), Stephen Gallagher wrote:
On 06/20/2016 03:09 PM, Jakub Hrozek wrote:
> On Mon, Jun 20, 2016 at 08:54:18PM +0200, Lukas Slebodnik wrote:
>> ehlo,
>>
>> Attached is a sligtly modified version of Michal's patch.
>
> The same patch is attached twice. Was it by accident or did you mean to
> send two patches?
>
Just single patch.
>> I fixed few coding style issues + added missing creation of
directory
>> + spec file change.
>
> You should have sent fixups to get credit, but meh :) Thanks for doing
> the work nonetheless.
>
It were very small changes which does not deserve separate patch :-)
I added my sign-off.
>>
>> You might notice that Michal removed detection of sssd.conf modified time.
>> It is because mtime could be obtiained from sssd.conf before parsing.
>> However, snippets files are open after parsing sssd.conf and mtime
>> of snippet files is ignored in the process.
>>
>> We have few options.
>> * check mtime directly in sssd
>> * add new function to libini_config to get latest mtime before parsing
>> (max_mtime(main.conf + alowed snippet files)
>> // it's little bit a complication for user of libini_config
>> // because user will need to paste regex for allowed snippets twice
>> // 1st time in new function for checking mtime and 2nd time in function
>> // ini_config_augment
>> * modify libini_config to set max mtime while parsing snippet files
>> // but we will need to parse files anyway. So I'm not sure what will be
>> // benefit of cehcking mtime after parsing.
>> * last option is to ignore mtime. (Michal's current version)
>> // and remove FIXME :-)
>
> Is there actually any downside to /always/ reading the config file and
> always creating the confdb from scratch? I would say that sssd restarts
> are a rare operation and the parsing and writes are not too big to slow
> down the startup significantly.
>
> I think the whole mtime logic was there only to allow online config
> changes, which is something we tried in the past, but could never code
> it up properly.
I let Michal's version untouched with FIXME in code.
Maybe, we will fix it sometimes.
And we can say that ticket
https://fedorahosted.org/sssd/ticket/3020
is fixed :-)
>
>
I think as long as we keep the parsing time very short, this is probably not an
issue. However, if we ever move SSSD to a socket-activated behavior, we will
want this to be very fast (or otherwise skippable).
It might be more complicated with socket activation.
ATM, the monitor is responsible for parsing sssd.conf and creating conf.db
But there could be a race condition with many socket activated responders.
Maybe we will switch to pure libini version of config (without confdb)
But that's far future :-)
>>
>> The main purpose of this mail is to decide wheteer we want change in ding-libs
>> or no.
>>
>> BTW. We cannot change directory for snippet files from command line.
>> Do we want such feature?
>> [root@graviton ~]# /usr/sbin/sssd --help
>> Usage: sssd [OPTION...]
>> -d, --debug-level=INT Debug level
>> -f, --debug-to-files Send the debug output to files instead of
>> stderr
>> --debug-timestamps=INT Add debug timestamps
>> --debug-microseconds=INT Show timestamps with microseconds
>> -D, --daemon Become a daemon (default)
>> -i, --interactive Run interactive (not a daemon)
>> -c, --config=STRING Specify a non-default config file
>
> Can you think of any use for this option? There can be only one sssd on
> the system, so I actually wonder if we can remove it..
>
I think we originally added this thinking of a future in which we might want to
be able to run a live SSSD for automated tests, but we obviously went another
route with it.
It might be handy when we will get rid of custom build of sssd in cwrap
integration tests.
The only use I can think of for this would be if someone actually
wanted to move
their sssd.conf to a mounted read-only location (since IIRC we actually
explicitly disallow sssd.conf as a symlink).
I will not add new option now.
It looks like there is an agreement that we needn't change anything in
libini_config for this use-case. So I will focus on other ding-libs patches.
BTW I need to update manual pages to reflect this change.
LS