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

Lukas Slebodnik lslebodn at redhat.com
Fri Apr 25 09:56:38 UTC 2014


On (25/04/14 11:43), Jakub Hrozek wrote:
>On Wed, Apr 23, 2014 at 11:04:54PM +0200, Lukas Slebodnik wrote:
>> 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
>
>Sure, did you only want to push Benjamin's patch
>(717008c8c3f29f3a1a77266cc72a6cfa616bf295 in master) or the whole set?
just Benjamin's patch. Other patches will probably not apply to 1.11 branch
and they were necessary mostly for dlopen test.

LS



More information about the sssd-devel mailing list