[SSSD] [PATCH] BUILD: Add version symbol files for public libraries.

Lukas Slebodnik lslebodn at redhat.com
Wed Jul 9 13:19:29 UTC 2014


On (09/07/14 14:31), Sumit Bose wrote:
>On Wed, Jul 09, 2014 at 01:15:17PM +0200, Jakub Hrozek wrote:
>> On Tue, Jul 08, 2014 at 11:04:27PM +0200, Lukas Slebodnik wrote:
>> > On (08/07/14 15:19), Sumit Bose wrote:
>> > >On Tue, Jul 08, 2014 at 12:51:48PM +0200, Jakub Hrozek wrote:
>> > >> On Tue, Jul 08, 2014 at 11:21:09AM +0200, Lukas Slebodnik wrote:
>> > >> > On (08/07/14 09:27), Jakub Hrozek wrote:
>> > >> > >On Thu, Jun 26, 2014 at 10:31:27AM +0200, Lukas Slebodnik wrote:
>> > >> > >> ehlo,
>> > >> > >> 
>> > >> > >> attached patch fixes ticket #2194.
>> > >> > >> 
>> > >> > >> If you wan to know more about version script (version maps) here are links:
>> > >> > >> 
>> > >> > >> http://people.redhat.com/drepper/dsohowto.pdf
>> > >> > >>     (sections 2.2.5 .. 2.2.7, 3.4, 3.5)
>> > >> > >> https://www.gnu.org/software/gnulib/manual/html_node/LD-Version-Scripts.html
>> > >> > >> ftp://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_25.html
>> > >> > >> 
>> > >> > >> LS
>> > >> > >
>> > >> > >I verified with:
>> > >> > >    nm -D $libname | awk '$2 == "T" {print $3";"}' | sort
>> > >> > >that the functions exported by the library match the functions defined
>> > >> > >in the version file.
>> > >> > >
>> > >> > >The versions also match the SONAMEs.
>> > >> > >
>> > >> > >One question inline:
>> > >> > >
>> > >> > >> From 0779624cf96480fe3b62f5c881fb9b123d24a965 Mon Sep 17 00:00:00 2001
>> > >> > >> From: Lukas Slebodnik <lslebodn at redhat.com>
>> > >> > >> Date: Mon, 9 Jun 2014 14:38:31 +0200
>> > >> > >> Subject: [PATCH] BUILD: Add version symbol files for public libraries.
>> > >> > >> 
>> > >> > >> Version symbol files will help package systems to catch backward compatible
>> > >> > >> changes (newly added functions) into library.
>> > >> > >> 
>> > >> > >> Resolves:
>> > >> > >> https://fedorahosted.org/sssd/ticket/2194
>> > >> > >
>> > >> > >[...]
>> > >> > >
>> > >> > >> new file mode 100644
>> > >> > >> index 0000000000000000000000000000000000000000..bd3abb211120784494fe366ddd19b94d9b982657
>> > >> > >> --- /dev/null
>> > >> > >> +++ b/src/sss_client/idmap/sss_nss_idmap.exports
>> > >> > >> @@ -0,0 +1,17 @@
>> > >> > >> +SSS_NSS_IDMAP_0.0.1 {
>> > >> > >> +
>> > >> > >> +    # public functions
>> > >> > >> +    global:
>> > >> > >> +
>> > >> > >> +        sss_nss_getsidbyname;
>> > >> > >> +        sss_nss_getsidbyid;
>> > >> > >> +        sss_nss_getnamebysid;
>> > >> > >> +        sss_nss_getidbysid;
>> > >> > >> +
>> > >> > >> +    # exported for mocking
>> > >> > >
>> > >> > >I don't think the comment is right. The symbol is exported because
>> > >> > >libsss_nss_idmap links with the client code:
>> > >> > >740 libsss_nss_idmap_la_SOURCES = \
>> > >> > >741     src/sss_client/idmap/sss_nss_idmap.c \
>> > >> > >742     src/sss_client/common.c \
>> > >> > >743     src/util/strtonum.c
>> > >> > >
>> > >> > >And libsss_nss_idmap uses the function to call out to the NSS responder
>> > >> > >(see src/sss_client/idmap/sss_nss_idmap.c:100)
>> > >> > >
>> > >> > >Can we not mark the function as global? It's not part of the sss_nss_idmap's
>> > >> > >API.
>> > >> > Try to comment out this symbol from file
>> > >> > --- a/src/sss_client/idmap/sss_nss_idmap.exports
>> > >> > +++ b/src/sss_client/idmap/sss_nss_idmap.exports
>> > >> > @@ -9,7 +9,7 @@ SSS_NSS_IDMAP_0.0.1 {
>> > >> >          sss_nss_getidbysid;
>> > >> >  
>> > >> >      # exported for mocking
>> > >> > -        sss_nss_make_request;
>> > >> > +    #    sss_nss_make_request;
>> > >> >  
>> > >> >      # everything else is local
>> > >> >      local:
>> > >> > 
>> > >> > sss_nss_idmap will fail, because function sss_nss_make_request cannot be mocked
>> > >> > sh-4.2$ cat sss_nss_idmap-tests.log
>> > >> > [==========] Running 1 test(s).
>> > >> > [ RUN      ] test_getsidbyname
>> > >> > 0x2 != 0
>> > >> > ../sssd/src/tests/cmocka/sss_nss_idmap-tests.c:102: error: Failure!
>> > >> > [  FAILED  ] test_getsidbyname
>> > >> > [==========] 1 test(s) run.
>> > >> > [  PASSED  ] 0 test(s).
>> > >> > [  FAILED  ] 1 test(s), listed below:
>> > >> > [  FAILED  ] test_getsidbyname
>> > >> > 
>> > >> > We can build libsss_nss_idmap twice.
>> > >> >     1) real library with version symbol file
>> > >> >     2) library without version symbol file only for tests (sss_nss_idmap-tests)
>> > >> > 
>> > >> > I was not able to find better solution.
>> > >> > 
>> > >> > LS
>> > >> 
>> > >> I tried playing a bit with Makefile.am and the only viable solution I
>> > >> see is to hardcode the list of libsss_nss_idmap sources in Makefile.am's
>> > >> sss_nss_idmap_tests_SOURCES except sss_client/common.c and mock
>> > >> sss_nss_lock and sss_nss_unlock..
>> > >> 
>> > >> But I'm not sure it's worth it, the rist is that a 3rd party links with
>> > >> sss_nss_make_request (which has no declaration in the public headers).
>> > >> 
>> > >> So now I prefer to push your original patch, but I would also like to
>> > >> hear other opinions, I'm not the biggest packaging expert around..
>> > >
>> > >While discussing this on irc with Andreas (cmocka author) and Jakub we
>> > >came up with
>> > >
>> > >SSS_NSS_IDMAP_0.0.1 {
>> > >
>> > >    # public functions
>> > >    global:
>> > >
>> > >        sss_nss_getsidbyname;
>> > >        sss_nss_getsidbyid;
>> > >        sss_nss_getnamebysid;
>> > >        sss_nss_getidbysid;
>> > >
>> > >    # everything else is local
>> > >    local:
>> > >        *;
>> > >};
>> > >
>> > >UNIT_TESTING_ONLY {
>> > >    global:
>> > >        sss_nss_make_request;
>> > >};
>> > >
>> > >which would move the sss_nss_make_request into a separate version which
>> > >is not for public use. RPMs will still show a Provides like:
>> > >
>> > >libsss_nss_idmap.so.0(UNIT_TESTING_ONLY)
>> > >
>> > >which can be filtered out (at least on recent Fedora version) with
>> > >
>> > >%global __provides_exclude ^.*FOR_CMOCKA.*$
>> > >
>> > >in the spec file.
>> > This change would only hide provides from rpm, which is not platform
>> > independent. I tried build libsss_nss_idmap.so in different way for
>> > test and for installation with custom make targets (check-local,
>> > install-exec-local, install-exec-hook). It was not possible because
>> > I would need targets which are executed before check or install and int the
>> > middle or after targets (check or install).
>> > 
>> > I decided to build library libsss_nss_idmap twice.
>> >     1) libsss_nss_idmap_test.so --will be used for sss_nss_idmap-test
>> >     2) libsss_nss_idmap.so --will be installed
>> > 
>> > Two version script files were combined for libsss_nss_idmap_test.so
>> > defualt sss_nss_idmap.exports and extra sss_nss_idmap.unit_tests, which
>> > contains function sss_nss_make_request.
>> > 
>> > LS
>> 
>> Originally I was going to say this breaks the purpose of unit tests that
>> we should stay as close to the production binaries as possible, but the
>> different is only in the version file (and rpath) and the
>> libsss_nss_idmap_la_* variables are used as a basis of the test library.
>> 
>> Provides also look sane,  nm output does, too.
>> 
>> I'm giving my ACK now, but I'll wait a bit before I push the patch in
>> case other developers didn't agree.
>
>I agree, Lukas took care that the library used in the unit tests is as
>similar to the production one as possible by putting everything what is
>possible into variables. I think it is ok to proceed this way for the
>time being. Nevertheless it might be a good idea to open a ticket for
>e.g. 1.13 as a reminder to try and find a solution. Maybe the binutils
>maintainers have something in their bag of tricks which might help here.
>
Yesterday, I played with some commands (strip, objdup, readelf).
It is possible to reduce symbols in symbol table '.symtab', but I was not able
to modify symbol table '.dynsym'. It is possible to remove hole section
'.dynsym', but it is absurd solution for dynamic library.

Some usefull commands from binutils.
  readelf -s .libs/libsss_nss_idmap.so
  readelf -S .libs/libsss_nss_idmap.so
  strip -R '.dynsym' .libs/libsss_nss_idmap.so
  strip --strip-all .libs/libsss_nss_idmap.so
  nm --defined-only --dynamic .libs/libsss_nss_idmap.so

I also tried to generate/modify version script files with make target hooks
I was not able to prepare acceptable solution.

I can file a ticket but it is very likely it will be moved to deferred bucket.

LS



More information about the sssd-devel mailing list