On Wed, Jul 09, 2014 at 03:14:31PM +0200, Michal Židek wrote:
On 07/09/2014 11:43 AM, Jakub Hrozek wrote:
>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.
Thanks for the review, pushed to master:
6b57784f0f175275fd900eca21c77415e3a5ea52
9a990aa9f7e8c105e0cfeea8d8cbdc776c2d5d7a
Michal
btw feel free to open the ticket you mentioned in office today about
better (more easier) configuration of the debugging..