On Tue, Jul 08, 2014 at 09:53:50AM +0200, Sumit Bose wrote:
> On Mon, Jul 07, 2014 at 10:20:21PM +0200, Jakub Hrozek wrote:
>> On Mon, Jul 07, 2014 at 03:57:28PM +0200, Sumit Bose wrote:
>>> On Mon, Jul 07, 2014 at 03:21:54PM +0200, Jakub Hrozek wrote:
>>>> On Tue, Jun 03, 2014 at 09:53:13AM +0200, Jakub Hrozek wrote:
>>>>> On Mon, Feb 24, 2014 at 08:39:25AM -0500, Simo Sorce wrote:
>>>>>> On Mon, 2014-02-24 at 11:54 +0100, Jakub Hrozek wrote:
>>>>>>> On Thu, Feb 20, 2014 at 08:47:30AM -0500, Stephen Gallagher
wrote:
>>>>>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>>>>>> Hash: SHA1
>>>>>>>>
>>>>>>>> On 02/19/2014 05:18 PM, Lukas Slebodnik wrote:
>>>>>>>>> On (19/02/14 22:03), Sumit Bose wrote:
>>>>>>>>>> On Wed, Feb 19, 2014 at 09:04:48PM +0100, Lukas
Slebodnik wrote:
>>>>>>>>>>> On (19/02/14 12:37), Jakub Hrozek wrote:
>>>>>>>>>>>> On Wed, Feb 19, 2014 at 12:02:23PM +0100,
Jakub Hrozek
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>> I realized I made a mistake in
the rebasing above. I
>>>>>>>>>>>>>> fixed that and then needed to do
another manual rebase of
>>>>>>>>>>>>>> these patches atop it. These
patches apply atop the
>>>>>>>>>>>>>> "[SSSD] DEBUG macro
refactoring v6" patches.
>>>>>>>>>>>>>
>>>>>>>>>>>>> These patches work for me, so ACK.
>>>>>>>>>>>>
>>>>>>>>>>>> Pushed to master.
>>>>>>>>>>>
>>>>>>>>>>> find_uid-tests failed with these patches on
fedora20.
>>>>>>>>>>>
http://kojipkgs.fedoraproject.org//work/tasks/8367/6548367/build.log
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>> Do you have additional patches in this build?
>>>>>>>>>>
>>>>>>>>>>
/builddir/build/SRPMS/sssd-1.11.90-0.20140219.1816.git22a9323._temp.fc20.src.rpm
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>> The git hash does not look like anything from master.
>>>>>>>>>>
>>>>>>>>> Yes, I was testing some patches.
>>>>>>>>>
>>>>>>>>>> I started a koji build
>>>>>>>>>>
http://koji.fedoraproject.org/koji/taskinfo?taskID=6549081 with
>>>>>>>>>> current master
(4cde267bec52ae1723a125d19439a5c75b47ebb7) which
>>>>>>>>>> is working fine.
>>>>>>>>>>
>>>>>>>>> I tried one more time with master and koji works
fine.
>>>>>>>>> Unfortunately, I am able to reproduce it locally in
mock :-(
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I've seen that happen intermittently as well in mock
and koji. I
>>>>>>>> suspect there may be a bug/race condition in
sd_uid_get_sessions(),
>>>>>>>> but I can't reproduce it consistently (and the DEBUG
error doesn't
>>>>>>>> seem to fire...)
>>>>>>>>
>>>>>>>> A quick look at the code suggests that we're probably
missing a
>>>>>>>> 'return EOK' after '*result = false',
though.
>>>>>>>>
>>>>>>>> I'll run a couple tests and see if that fixes the
issue...
>>>>>>>
>>>>>>> I found another issue related to journald support..if you
compile sssd
>>>>>>> with journald support and run it on the foreground (-i) then
the debug
>>>>>>> messages are still redirected to journal. That seems strange
to me, but
>>>>>>> also in line with what a comment in the code says:
>>>>>>>
>>>>>>> /* If we are not outputting logs to files, we
should be sending them
>>>>>>> * to journald.
>>>>>>> * NOTE: on modern systems, this is where
>>>>>>> * stdout/stderr will end up
>>>>>>>
>>>>>>> So I'm not entirely sure about the expected behaviour,
but I would
>>>>>>> prefer if there was still a way to see the debug messages
directly on
>>>>>>> the console.
>>>>>>>
>>>>>>> I hacked up a patch that also adds a third state where if
sssd is running
>>>>>>> on the foreground even with journald support, then just
fprintf is used.
>>>>>>> See the attachment.
>>>>>>
>>>>>> Wouldn't it be simpler to just set debug_file = stderr ?
>>>>>
>>>>> I tried to go this way, but it seems actually harder to me..
>>>>>
>>>>> To summarize:
>>>>> with "sssd -f" debug_file should be a log file
>>>>> with "sssd" debug_file should be NULL, indicating journald
>>>>> with "sssd -i", debug_file should be stderr, indicating
stderr.
>>>>>
>>>>> The problem is, that we don't pass the "-i" flag down
to the responders
>>>>> and back ends when forking them. So the alternative would be to add
"-i"
>>>>> to sssd_be and all responders, pass the flag to the child processes
and
>>>>> only set debug_file to stderr if '-i' is present. If you
prefer this way,
>>>>> I can prepare a patch along these lines.
>>>>>
>>>>> But then we also terminate the process which is running with
"-i" if its
>>>>> stdin goes away, I guess it would be safe to keep this logic since
stdin
>>>>> in the child processes is /dev/null already, I just wanted to
mention
>>>>> there is a number of cases to test..
>>>>
>>>> Any opinions? I'd like to fix this bug before tagging 1.12.0..
>>>
>>> Maybe having an option --debug-to-stdout would be nicer for the child
>>> processes because it fits better to the existing SSSD_DEBUG_OPTS and
>>> will have no additional meaning as -i? For the monitor this option
>>> should be rejected and hidden from the help.
>>>
>>> bye,
>>> Sumit
>>
>> Perhaps..
>>
>> to clarify, you're suggesting that --debug-to-stdout would be added to
>> all processes (monitor and child), but only child processes in libexec
>> would expose it and this option would be used when spawning the child
>> processes?
>
> yes, my idea was to add it to SSSD_DEBUG_OPTS which would make it
> immediately available to all processes. But having it for the monitor
> make no sense because '-D --debug-to-stdout' and '-f
--debug-to-stdout'
> are config errors and '-i --debug-to-stdout' is a no-op.
>
> bye,
> Sumit
Patches are attached. I found out the same issue also affected the CLI
tools and tests, which is fixed with the second patch.
In order to test, you need to run configure --with-initscript=systemd
--with-systemdunitdir=/usr/lib/systemd/system --with-syslog=journald
sssd -i -d <N> logs to stderr
sssd -i -f -d <N> logs to files
sssd -D -d <N> logs to journald
sssd -D -f -d <N> logs to files
It's not possible to log to journald when running on the foreground or
to stderr when running as deamon.
I think the behaviour with these patches makes sense and
works as expected. Ack.
Michal