[SSSD] [PATCH] BUILD: Link libsss_krb5_common.so to libkeyutils.so

Lukas Slebodnik lslebodn at redhat.com
Wed Apr 23 21:04:54 UTC 2014


On (16/04/14 11:12), Jakub Hrozek wrote:
>On Tue, Apr 15, 2014 at 11:02:15AM +0200, Jakub Hrozek wrote:
>> On Mon, Mar 17, 2014 at 09:34:04AM +0100, Lukas Slebodnik wrote:
>> > On (17/03/14 07:53), Lukas Slebodnik wrote:
>> > >On (15/03/14 17:57), Lukas Slebodnik wrote:
>> > >>On (12/11/13 11:15), Simo Sorce wrote:
>> > >>>On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
>> > >>>> On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
>> > >>>
>> > >>>> > Updated patches are attached.
>> > >>>> 
>> > >>>> This is not the right way to fix the failure.
>> > >>>> 
>> > >>>> That library *knowingly* has missing symbols because it is an internal
>> > >>>> library.
>> > >>>> 
>> > >>>> You need to build or link to an outer program that have only the
>> > >>>> necessary symbols you know are going to be available.
>> > >>>> 
>> > >>>> In this case those symbols are in libsss_krb5.so, so all you should nee
>> > >>>> to do is to have this in tests:
>> > >>>> 
>> > >>>> +    { "libsss_krb5_common.so", { LIBPFX"libdlopen_test_providers.so",
>> > >>>> +                                 LIBPFX"libsss_krb5_common.so",
>> > >>>> + 				  LIBPFX"libsss_krb5.so",
>> > >>>> + 				  NULL } },
>> > >>>> 
>> > >>>> Adding c files to librraies to "make them pass" the dl-open test is
>> > >>>> wrong in 99% of the cases. you are just making the test pass not testing
>> > >>>> that the libraries have the right dependencies. In particular for
>> > >>>> internal modules you really need to understand the hierarchy of the
>> > >>>> modules and link in the test all the necessary dependencies before
>> > >>>> thinking of adding any file to the module itself.
>> > >>>> 
>> > >>>> In this case what you did was to duplicate symblos in the resulting Text
>> > >>>> file, and that can potentially cause very serious consequencies if any
>> > >>>> of those symbols is a static symbols that is meant to have a share de
>> > >>>> value all code rely on.
>> > >>>
>> > >>>Oh let me add.
>> > >>>I did my check very quickly so it is possible that my version is not the
>> > >>>final correct approach, I was just describing how you should pass in
>> > >>>multiple libs for dependency.
>> > >>>
>> > >>>It is possible that you need those symbols directly in
>> > >>>libsss_krb5_common.so, but if that is actually the case, then it means
>> > >>>you have to remove those symbols from libsss_krb5.so and add them
>> > >>>libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and
>> > >>>tests. This may even require moving functions from one file to another
>> > >>>in the extreme case.
>> > >>>
>> > >>
>> > >>zombie patches are returning :-)
>> > >>
>> > >>I hope all commit messages are self descripting.
>> > >>
>> > >>They were lot of changes in Makefile.am
>> > >>sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am
>> > >> Makefile.am | 99 +++++++++++++++++++------------------------------------------
>> > >> 1 file changed, 31 insertions(+), 68 deletions(-)
>> > >>
>> > >>LS
>> > >
>> > >>From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001
>> > >>From: Lukas Slebodnik <lslebodn at redhat.com>
>> > >>Date: Sat, 15 Mar 2014 17:29:25 +0100
>> > >>Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to
>> > >> libsss_ldap_common.so
>> > >>
>> > >
>> > >self-NACK
>> > >
>> > >make distcheck fails with 5th patch.
>> > >
>> > Interdiff for 5th patch:
>> > 
>> > diff --git a/Makefile.am b/Makefile.am
>> > index 2d0a295..a65369b 100644
>> > --- a/Makefile.am
>> > +++ b/Makefile.am
>> > @@ -1742,6 +1742,10 @@ endif
>> >  # Plugin Libraries #
>> >  ####################
>> >  
>> > +# libsss_krb5_common must be installed before libsss_ldap_common
>> > +# because libtool tries to relink libsss_ldap_common when installing
>> > +# libsss_ldap_common and therefore make distcheck fails
>> > +pkglib_LTLIBRARIES += libsss_krb5_common.la
>> >  pkglib_LTLIBRARIES += libsss_ldap_common.la
>> >  libsss_ldap_common_la_SOURCES = \
>> >      src/providers/ldap/ldap_id.c \
>> > @@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \
>> >      src/providers/ldap/sdap.c \
>> >      src/util/user_info_msg.c \
>> >      src/util/sss_ldap.c
>> >  libsss_ldap_common_la_LIBADD = \
>> >      $(KRB5_LIBS) \
>> >      libsss_krb5_common.la \
>> > @@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \
>> >      src/providers/ldap/sdap_async_autofs.c
>> >  endif
>> >  
>> > libsss_ldap_common_la_CFLAGS = \
>> >     $(KRB5_CFLAGS)
>> > 
>> >  
>> > -pkglib_LTLIBRARIES += libsss_krb5_common.la
>> >  libsss_krb5_common_la_SOURCES = \
>> >      src/providers/krb5/krb5_utils.c \
>> >      src/providers/krb5/krb5_become_user.c \
>> > 
>> > Updated patches are attached
>> > 
>> > LS
>> 
>> Sorry for the delay in reviewing the packages.
>> 
>> I think these packages are good and I'd like to ACK them.
>> 
>> The testing consisted of:
>>     * visual inspection of the patches. The commit messages help,
>>       thanks!
>>     * applying the dlopen-test patch on master. The dlopen test then
>>       didn't pass. After aplying the other patches, the dlopen test
>>       passed as well.
>>     * starting IPA, AD and LDAP (with and without ID mapping to be sure)
>>       went fine
>
>Pushed all patches to master in:
>    a25942bc2f6ac9b3b2817ede35fa2b445373c6e4
>    cc1c033c34b5f816b633d27a21aefbf811a7cf72
>    6261893e00bd14fdd192ffc9a1379cb9c647d326
>    12805da52a93c268290cec7b8fbbdbd4ea8abc3e
>    7fc27c7a3ccbb6aecb8cf4a4a5f91962028cb897
>    717008c8c3f29f3a1a77266cc72a6cfa616bf295

Could we push Benjamin's patch to 1.11 branch?

LS



More information about the sssd-devel mailing list