On Thu, Nov 07, 2013 at 08:43:35PM +0100, Lukas Slebodnik wrote:
On (31/10/13 11:12), Lukas Slebodnik wrote:
>On (15/10/13 15:42), Benjamin Franzke wrote:
>>Hi Lukas,
>>
>>
>>2013/10/15 Lukas Slebodnik <lslebodn(a)redhat.com>
>>
>>> On (15/10/13 11:47), Benjamin Franzke wrote:
>>> >Hi,
>>> >
>>> >These two patches add missing CFLAGS/LIBS to Makefile.am:
>>> >
>>> >[PATCH 1/2] BUILD: Link libsss_ad.so to sasl libs
>>> >[PATCH 2/2] BUILD: Use OPENLDAP_CFLAGS instead of LDAP_CFLAGS
>>> ACK to 2nd patch
>>> >
>>> >This underlinking was noticed in make check (dlopen-test).
>>> >
>>> >Note:
>>> >It failed for me since my openldap build had no sasl support,
>>> >which would otherwise have pulled in libsasl2.so.
>>> >Of course, that support should be in place, but the linking should
still
>>> be
>>> >fixed.
>>> >
>>> >BTW: It would propably be nice to have a configure check whether
>>> >openldap has sasl support, but it seems that would need a check if
>>> >ldap_sasl_interactive_bind returns LDAP_NOT_SUPPORTED.
>>> >
>>> >Regards, Ben
>>>
>>> I have a little problem with the first patch.
>>>
>>>
>>> From d26a15de098df2b42582cda590e184c69f48bb7f Mon Sep 17 00:00:00 2001
>>> From: Benjamin Franzke <benjaminfranzke(a)googlemail.com>
>>> Date: Tue, 15 Oct 2013 10:27:36 +0200
>>> Subject: [PATCH 1/2] BUILD: Link libsss_ad.so to sasl libs
>>>
>>> This is for the sasl_client_init symbol.
>>> Introducted in commit fb945a2c.
>>> ---
>>> Makefile.am | 2 ++
>>> configure.ac | 2 +-
>>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index
>>>
ff1e71e72d90a6eff658a40e2ca24c1929b31aa5..8c919d40481720e4661a42a188cea2fd179282d4
>>> 100644
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -1713,12 +1713,14 @@ libsss_ad_la_CFLAGS = \
>>> $(AM_CFLAGS) \
>>> $(SYSTEMD_LOGIN_CFLAGS) \
>>> $(LDAP_CFLAGS) \
>>> + $(SASL_CFLAGS) \
>>> $(DHASH_CFLAGS) \
>>> $(KRB5_CFLAGS) \
>>> $(NDR_NBT_CFLAGS)
>>> libsss_ad_la_LIBADD = \
>>> $(SYSTEMD_LOGIN_LIBS) \
>>> $(OPENLDAP_LIBS) \
>>> + $(SASL_LIBS) \
>>> $(DHASH_LIBS) \
>>> $(KEYUTILS_LIBS) \
>>> $(KRB5_LIBS) \
>>> diff --git a/configure.ac b/configure.ac
>>> index
>>>
d28d55f3b0eb35cc4ecc02ae9b1fafbcb9588dcf..7e3819a86ad94176e95f1ded07b88d25399888de
>>> 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -260,7 +260,7 @@ fi
>>>
>>> AM_CHECK_INOTIFY
>>>
>>> -AC_CHECK_HEADERS([sasl/sasl.h],,AC_MSG_ERROR([Could not find SASL
>>> headers]))
>>> +PKG_CHECK_MODULES([SASL], [libsasl2], [], [AC_MSG_ERROR([Could not find
>>> SASL library])])
>>>
>>> pkg-config file needn't be available one some distribution (platforms).
>>> For example popt-devel on fedora 19 doesn't have pkg-config file.
>>> So it is better to fallback to AC_CHECK_HEADER.
>>>
>>> 1. And it does not mean that openldap was build with sasl support if
>>> libsasl2
>>> is installed on the machine. Detection should more complex.
>>>
>>
>>Yes, thats why the dea to detect whether ldap has sasl support, which is
>>not easy, as Stephen already said.
>>
>>>
>>> 2. According to man ldap_sasl_interactive_bind_s, we should only link with
>>> library "OpenLDAP LDAP (libldap, -lldap)" and we does not
directly use
>>> any sasl function. What kind of distribution do you use?
>>>
>>
>>As written in the commit message, since commit fb945a2c sssd uses sasl
>>unconditionally:
>>git grep sasl_client_init
>>src/providers/ad/ad_init.c: (void)sasl_client_init(ad_sasl_callbacks);
>>
>>So if sssd should be build without sasl that'd need to be fixed.
>>
>>>
>>> 3. IIRC, sssd can be build with ldap without sasl support, but ipa provider
>>> will not work. And it should be similar with ad provider. I would prefer
>>> to use conditional build instead of failing if ldap does not support
>>> sasl.
>>> Because with your patch, sssd could not be build without sasl library
>>> and
>>> someone can use sssd only with ldap provider (and he doesn't care
about
>>> sasl)
>>>
>>
>>How did it not fail before, when sasl was not installed? I mean there was
>>error too..
>>
>I realized sssd cannot be built without header file sasl.h, because
>we include header file in sdap_async_connection.c
> src/providers/ldap/sdap_async_connection.c:#include <sasl/sasl.h>
>But we needn't link ldap_common with sasl library, because sasl functions
>are not used in module sdap_async_connection.c
>
>The last problem is in your patch, that you replaced
>macro AC_CHECK_HEADERS with pkg-config detection. Sasl pkg-config file
>needn't be available on some distributions.
>For example, the latest ubuntu does not have sasl.pc in the package
>libsasl2-dev.
>
>http://packages.ubuntu.com/saucy/amd64/libsasl2-dev/filelist
>
>LS
I am attaching patches with more robust detection of SASL,
because pkg-config file is not available in older versions of
cysrus sasl <= 2.1.25. (Fedora<=18, RHEL<=6, all versions of ubuntu)
Configure failed with Benjamin's version on RHEL6.
LS
These patches look good to me and are mostly an incremental improvement
over what Stephen already acked before, so ACK from me.