On (24/11/14 10:47), Jakub Hrozek wrote:
>On Mon, Nov 24, 2014 at 09:51:58AM +0100, Lukas Slebodnik wrote:
>> On (22/11/14 17:25), Jakub Hrozek wrote:
>> >On Sat, Nov 22, 2014 at 05:12:09PM +0100, Jakub Hrozek wrote:
>> >> On Tue, Nov 18, 2014 at 12:25:40PM +0100, Lukas Slebodnik wrote:
>> >> > On (18/11/14 12:22), Lukas Slebodnik wrote:
>> >> > >On (13/11/14 10:08), Simo Sorce wrote:
>> >> > >>On Thu, 13 Nov 2014 10:44:45 +0100
>> >> > >>Jakub Hrozek <jhrozek(a)redhat.com> wrote:
>> >> > >>
>> >> > >>> On Wed, Nov 12, 2014 at 08:04:46PM -0500, Simo Sorce
wrote:
>> >> > >>> > On Wed, 12 Nov 2014 16:36:00 +0100
>> >> > >>> > Lukas Slebodnik <lslebodn(a)redhat.com>
wrote:
>> >> > >>> >
>> >> > >>> > > On (12/11/14 10:00), Simo Sorce wrote:
>> >> > >>> > > >I would create a helper function to be
called on return that
>> >> > >>> > > >transforms the error accordingly. This
will allow to write the
>> >> > >>> > > >code _and_ the comment once.
>> >> > >>> > > >
>> >> > >>> > > In this case, Stephan's patch is
better
>> >> > >>> > >
https://bugzilla.redhat.com/attachment.cgi?id=788567
>> >> > >>> >
>> >> > >>> > Yes, this is a valid alternative.
>> >> > >>> >
>> >> > >>> > > >The comment should be changed to
something like this in either
>> >> > >>> > > >case: /* When sssd is stopped return a
safe error code as if sss
>> >> > >>> > > >was not configured at all in nsswitch.
This prevents bogus
>> >> > >>> > > >errors from causing issues in
applications, before sssd starts
>> >> > >>> > > >or if it fails to respond. */
>> >> > >>> > > >
>> >> > >>> > > >No need to mention that sss is by
default in nsswitch, as it is
>> >> > >>> > > >not in all distributions and it really
is inconsequential, the
>> >> > >>> > > >same behaviour change hleps when sss is
not the default but is
>> >> > >>> > > >has been manually added and sssd is
stopped or not started yet
>> >> > >>> > > >(for example during boot).
>> >> > >>> > > nss-pam-ldapd has the same behaviour in the
same situation.
>> >> > >>> > > Will we patch it as well? It's very
likely we won't.
>> >> > >>> >
>> >> > >>> > Sorry, I do not see how that matters :)
>> >> > >>> >
>> >> > >>> > > The biggest problem is that sss is by
default in nsswitch on
>> >> > >>> > > fedora/rhel>=7 due to glibc caching and
problem with GNOME,
>> >> > >>> > > a) sssd-client is installed by default on
this platforms.
>> >> > >>> > > b) sssd need't be configured by default
and in most cases won't be
>> >> > >>> > > => sssd cannot run
>> >> > >>> > > c) glibc developers don't want to
adjust the error return code in
>> >> > >>> > > glibc
>> >> > >>> > >
>> >> > >>> > > As a result of this, we need to patch
sssd.
>> >> > >>> > > I would say we should patch sssd just in
downstream and
>> >> > >>> > > Stephan's patch works well. I tested
it.
>> >> > >>> >
>> >> > >>> > Ok, then let's go with Stephen's patch.
>> >> > >>> >
>> >> > >>> > Simo.
>> >> > >>> >
>> >> > >>>
>> >> > >>> I don't personally mind whether we use Lukas'
or Stephen's patch but I
>> >> > >>> would /strongly/ prefer the patch to be pushed
upstream.
>> >> > >>>
>> >> > >>> Non-upstream patches should be considered valid only
as a temporary
>> >> > >>> workarounds..
>> >> > >>
>> >> > >>I was not aware the patch was not proposed for upstream. I
strongly
>> >> > >>disagree on this kind of patches being platform specific
too.
>> >> > >>
>> >> > >New version of patch is attached.
>> >> > >
>> >> > Ups, one more time.
>> >> >
>> >> > LS
>> >>
>> >> > From eb617a980d50c7081ecb6eda0d489fc0eb8963f7 Mon Sep 17 00:00:00
2001
>> >> > From: Lukas Slebodnik <lslebodn(a)redhat.com>
>> >> > Date: Tue, 18 Nov 2014 12:02:10 +0100
>> >> > Subject: [PATCH] sss_client: Work around glibc bug
>> >> >
>> >> > glibc is inconsistent with how it treats and returns
NSS_STATUS_UNAVAIL.
>> >> >
>> >> > The sss nss plugin is present in nsswitch by default on some
platforms
>> >> > due to glibc caching and problem with long living applications
(e.g. GNOME).
>> >> > The SSSD nss plugin should behave as if it was functioning but had
no data even
>> >> > thought sssd is not running. The errors have to be passed from nss
plugin up to
>> >> > the user with minimal moidiffication.
>> >> >
>> >> > Thanks to Stephen Gallagher for initial patch.
>> >> >
>> >> > Resolves:
>> >> >
https://fedorahosted.org/sssd/ticket/2439
>> >> > ---
>> >> > src/conf_macros.m4 | 13 +++++++++++++
>> >> > src/sss_client/common.c | 10 ++++++++++
>> >> > 2 files changed, 23 insertions(+)
>> >> >
>> >> > diff --git a/src/conf_macros.m4 b/src/conf_macros.m4
>> >> > index
fbee81f56e484b618379f7c987ecee50ae48917e..bb283ef248ee017fd608bd85cc68666db0300fd9 100644
>> >> > --- a/src/conf_macros.m4
>> >> > +++ b/src/conf_macros.m4
>> >> > @@ -708,6 +708,19 @@ AC_ARG_ENABLE([dbus-tests],
>> >> > [build_dbus_tests=yes])
>> >> > AM_CONDITIONAL([BUILD_DBUS_TESTS], [test x$build_dbus_tests =
xyes])
>> >> >
>> >> > +AC_ARG_ENABLE([sss-default-nss-plugin],
>> >> > + [AS_HELP_STRING([--enable-sss-default-nss-plugin],
>> >> > + [This option change standard
behaviour of sss nss
>> >> > + plugin. If this option is enabled
the sss nss
>> >> > + plugin will behave as it was not
in
>> >> > + nsswitch.conf when sssd is not
running.
>> >> > + [default=no]])],
>> >> > + [],
>> >>
>> >> Didn't you mean to add:
>> >> enable_sss-default-nss-plugin=$enablevar
>> >>
>> >> here?
>> >>
>> >> Additionally, it would be nice to explain in the commit message why
the
>> >> feature is disabled -- explain the effect enabling by default would
have
>> >> on searches that don't find anything.
>> >
>> >Actually, you also need to convert dashes to underscores in the variable
>> >name. As the autoconf docs say:
>> > The option's argument is available to the shell commands
>> > action-if-given in the shell variable enableval, which is actually
>> > just the value of the shell variable named enable_feature, with any
>> > non-alphanumeric characters in feature changed into ‘_’.
>> >
>> >This change did the trick for me and I was able to test the return value
>> >in gdb.
>> In gdb? really
>> Could you ellaborate?
>
>Once I fixed the config option so the build-time switch actually worked:
>
I thought you was able to see value of bash variable
enable_sss-default-nss-plugin :-)
New version is attached.
LS
Thank you, the code looks good. Were you able to reproduce the exact
same error as the user saw, though?
I tested with "files sss ldap" in nsswitch.conf and SSSD was off. Even
without the patch, getent group $ipagroup was able to fall back to
ldap..so I guess I must be missing something.