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

Jakub Hrozek jhrozek at redhat.com
Fri Apr 25 09:43:01 UTC 2014


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?



More information about the sssd-devel mailing list