Re: [SSSD] [PATCHES] nss: add SSS_NSS_GETORIGBYNAME request

[Lukas Slebodnik]

On (14/10/14 17:20), Sumit Bose wrote:
> On Tue, Oct 14, 2014 at 04:30:15PM +0200, Lukas Slebodnik wrote:
> > On (14/10/14 11:09), Sumit Bose wrote:
> > >Hi,
> > >
> > >this patch adds a new interface to the nss responder which is similar to
> > >SSS_NSS_GETSIDBYNAME but can return more information about the given
> > >object than just the SID and which is not available with the POSIX
> > >interface as well. As mentioned in the commit messages the main use case
> > >is the extdom plugin on the FreeIPA server.
> > >
> > >bye,
> > >Sumit
> > 
> > >From f8500f00224e781b03ebab0ab396565e4c034489 Mon Sep 17 00:00:00 2001
> > >From: Sumit Bose <sbose(a)redhat.com&gt;
> > >Date: Thu, 9 Oct 2014 21:05:34 +0200
> > >Subject: [PATCH 2/2] sss_nss_idmap: add sss_nss_getorigbyname()
> > >
> > >This patch adds an interface to the new SSS_NSS_GETORIGBYNAME request of
> > >the nss responder to libsss_nss_idmap.
> > >
> > >The main use case for this new call is to replace sss_nss_getsidbyname()
> > >in the extdom plugin on the FreeIPA server to get more information about
> > >the given object than just the SID which is not available with the
> > >default POSIX interfaces.
> > >---
> > > src/sss_client/idmap/sss_nss_idmap.c       | 113 +++++++++++++++++++++++++++++
> > > src/sss_client/idmap/sss_nss_idmap.exports |   2 +
> > > src/sss_client/idmap/sss_nss_idmap.h       |  32 ++++++++
> > > src/tests/cmocka/sss_nss_idmap-tests.c     |  26 ++++++-
> > > 4 files changed, 172 insertions(+), 1 deletion(-)
> > >
> > //snip
> > 
> > >diff --git a/src/sss_client/idmap/sss_nss_idmap.exports
> b/src/sss_client/idmap/sss_nss_idmap.exports
> > >index 7b8488b..ba340ca 100644
> > >--- a/src/sss_client/idmap/sss_nss_idmap.exports
> > >+++ b/src/sss_client/idmap/sss_nss_idmap.exports
> > >@@ -7,6 +7,8 @@ SSS_NSS_IDMAP_0.0.1 {
> > >         sss_nss_getsidbyid;
> > >         sss_nss_getnamebysid;
> > >         sss_nss_getidbysid;
> > >+        sss_nss_getorigbyname;
> > >+        sss_nss_free_kv;
> > 
> > It works but it is not the right solution. The purpose of version symbol file
> > is to help linker distinguish between two version of libraries with backward
> > compatible changes. (just new functions were added).
> > Without version symbol file, linker cannot detect differences due to the same
> > soname and it need to be solved in downstream packaging.
> > 
> > sh$ objdump -p .libs/libsss_nss_idmap.so | grep SONAME
> >   SONAME               libsss_nss_idmap.so.0
> > 
> > The right approach is to add new version as in
> >     ./src/sss_client/libwbclient/wbclient.exports
> > The new version will extend previous one.
> > 
> > When I created version symbol files, I decided to use full version of library
> > generated from version-info. It could be any number, but it is easier
> > to backport changes if version is not related to sssd version.
> > 
> > Provisioned version with attached patch:
> >   sh$ objdump -p .libs/libsss_nss_idmap.so | grep -A5 "Version
> definitions"Version definitions:
> >   1 0x01 0x0b2b7380 libsss_nss_idmap.so.0
> >   2 0x00 0x0597c1a1 SSS_NSS_IDMAP_0.0.1
> >   3 0x00 0x0597c2a0 SSS_NSS_IDMAP_0.1.0
> >           SSS_NSS_IDMAP_0.0.1
> > 
> > Version symbol file add version to each function:
> >   sh$ objdump -T .libs/libsss_nss_idmap.so | grep SSS_NSS_IDMAP
> >   0000000000000000 g    DO *ABS*  0000000000000000  SSS_NSS_IDMAP_0.0.1
> SSS_NSS_IDMAP_0.0.1
> >   0000000000001930 g    DF .text  0000000000000052  SSS_NSS_IDMAP_0.1.0
> sss_nss_getorigbyname
> >   0000000000001420 g    DF .text  0000000000000052  SSS_NSS_IDMAP_0.1.0
> sss_nss_free_kv
> >   0000000000001830 g    DF .text  000000000000003f  SSS_NSS_IDMAP_0.0.1
> sss_nss_getsidbyid
> >   0000000000001870 g    DF .text  0000000000000052  SSS_NSS_IDMAP_0.0.1
> sss_nss_getnamebysid
> >   0000000000000000 g    DO *ABS*  0000000000000000  SSS_NSS_IDMAP_0.1.0
> SSS_NSS_IDMAP_0.1.0
> >   00000000000017d0 g    DF .text  0000000000000052  SSS_NSS_IDMAP_0.0.1
> sss_nss_getsidbyname
> >   00000000000018d0 g    DF .text  0000000000000058  SSS_NSS_IDMAP_0.0.1
> sss_nss_getidbysid
> > 
> > So linker will be able to distinguish between two libraries with the same
> > soname and automatically requires necessary versions.
> > 
> > >     # everything else is local
> > >     local:
> > 
> > and version-info should be bumped as well.
>
> Thank you Lukas I really forgot to update the version. While adding the
> new calls to the exports file I was thinking that I can fix the version
> later when I know that everything is working as expected.
>
> Would you mind to send your changes as a proper patch to this thread so
> that the attributions are correct?
>  
It is part of review. I don't want to have another patch. I just wanted to
simplify your job. Feel free to squash changes into your patches.

LS