Bug 1509063 - ABI change (missing functions) in libnfsidmap-2.2.1-0.rc1.fc28
Summary: ABI change (missing functions) in libnfsidmap-2.2.1-0.rc1.fc28
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: nfs-utils
Version: 27
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Steve Dickson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-11-02 20:33 UTC by Lukas Slebodnik
Modified: 2017-11-15 17:47 UTC (History)
5 users (show)

Fixed In Version: nfs-utils-2.2.1-1.rc1.fc27
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-11-15 17:47:02 UTC
Type: Bug


Attachments (Terms of Use)

Description Lukas Slebodnik 2017-11-02 20:33:47 UTC
Description of problem:
sssd provides a plugin for NFSv4 rpc.idmapd
https://koji.fedoraproject.org/koji/rpminfo?rpmID=11807492

And latest build of libnfsidmap in rawhide broke our unit tests
which was due to ABI change in library even thought soname is the same.


Version-Release number of selected component (if applicable):
sh$ rpm -q libnfsidmap
libnfsidmap-2.2.1-0.rc1.fc28.x86_64

How reproducible:
Deterministic

Steps to Reproduce:
1. objdump -T /usr/lib64/libnfsidmap.so.0.3.0 | grep conf_get_str
2. echo $?

Actual results:
sh# objdump -T /usr/lib64/libnfsidmap.so.0.3.0 | grep conf_get_str
sh# echo $?
1

Expected results:
sh# objdump -T /usr/lib64/libnfsidmap.so.0.3.0 | grep conf_get_str
00000000000040d0 g    DF .text  00000000000000ac  Base        conf_get_str_with_def
0000000000003f00 g    DF .text  0000000000000094  Base        conf_get_str


Additional info:
conf_* functions were not in public header file but they were part of ABI.
And functions were removed between 0.27-3.fc27 and 2.2.1-0.rc1.fc28
without changing soname.

Comment 1 Lukas Slebodnik 2017-11-02 20:36:41 UTC
Here is sssd unit test failure:
/home/build/sssd/src/tests/dlopen-tests.c:221:F:dlopen:test_dlopen_base:0: Error opening sss.so: [dlopen() failed: /home/build/sssd/ci-build-debug/.libs/sss.so: undefined symbol: conf_get_str]
ERROR dlopen-tests (exit status: 99)

I do not know how realistic it would be to return functions back
but it would be good to at leas t provide some alternative for function conf_get_str. Ideally something which would be public and even in header files

Comment 2 Steve Dickson 2017-11-03 11:33:20 UTC
We just roll the libnfsidmap code into nfs-utils and
it looks like all the conf_ routines got moved in
to an internal libnfsconf.a 

I guess I didn't realize those were published interfaces
I was thinking the only published interface were in nfsidmap.h

Comment 3 Lukas Slebodnik 2017-11-03 11:40:43 UTC
(In reply to Steve Dickson from comment #2)
> We just roll the libnfsidmap code into nfs-utils and
> it looks like all the conf_ routines got moved in
> to an internal libnfsconf.a 
> 
> I guess I didn't realize those were published interfaces
> I was thinking the only published interface were in nfsidmap.h

They were part of ABI but never part of API(header files)
therefore we had just forward declaration in internal header file
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/nfs/nfsidmap_internal.h#_78

BTW here is a code of plugin
https://pagure.io/SSSD/sssd/blob/master/f/src/sss_client/nfs

Could you suggest some alternative? Or provide properly namespaced public functions?

Anyway it is change of ABI so you should at least bump SONAME.

Comment 4 Alice Mitchell 2017-11-03 12:08:57 UTC
It looks like the original libnfsidmap leaked all sorts of functions and variables that are private and should never have been exposed, hence why they are not documented or listed in the header files.

Ironically some of the functions exposed this way i had proposed to make public in their own library, but received pushback because it would have created more ABI.

Some of these functions could be exposed again, like conf_get_*, but others have been changed to fix bugs and to improve thread safety.
e.g. the conf_path global has now gone, and been replaced by a parameter to conf_init()

Comment 5 Steve Dickson 2017-11-03 13:10:00 UTC
(In reply to Justin Mitchell from comment #4)
> 
> Some of these functions could be exposed again, like conf_get_*, but others
> have been changed to fix bugs and to improve thread safety.
> e.g. the conf_path global has now gone, and been replaced by a parameter to
> conf_init()
I really do not want to change the SONAME... Lets put as much back as we can...

Comment 6 Lukas Slebodnik 2017-11-03 21:15:50 UTC
BTW I am fine with different name for conf_get_* if you want to make them public.
And as you can see we use only "conf_get_str" from private-public interface :-)
And I know we used not really public functions. I am not the author of plugin.
The plugin was not written by sssd core team. It was san external contribution.

But I would like to have as cleanest upgrade path as possible.
So if you decide to rename, hide some functions then the cleanest solution is to bump SONAME. I could workaround it on rpm level even without SONAME change but other distributions would have the same problem.

(In reply to Justin Mitchell from comment #4)
> Some of these functions could be exposed again, like conf_get_*, but others

That would be sufficient for sssd plugin and would not break anything.

> have been changed to fix bugs and to improve thread safety.
> e.g. the conf_path global has now gone, and been replaced by a parameter to
> conf_init()

Do you know about other other plugins which might miss "conf_path" ?
If know then lets hide them :-)

Comment 7 Lukas Slebodnik 2017-11-03 21:18:48 UTC
And maybe related question.

Do you have an idea why there is a missing package libnfsidmap-devel in fedora 27? Because I cannot see removed it from koji/

DEBUG util.py:439:  Last metadata expiration check: 0:00:00 ago on Fri 03 Nov 2017 03:31:29 PM UTC.
DEBUG util.py:439:  Package findutils-1:4.6.0-14.fc27.x86_64 is already installed, skipping.
DEBUG util.py:439:  No matching package to install: 'libnfsidmap-devel'
DEBUG util.py:439:  Package libxml2-2.9.5-2.fc27.x86_64 is already installed, skipping.
DEBUG util.py:439:  Package pkgconf-pkg-config-1.3.9-1.fc27.x86_64 is already installed, skipping.
DEBUG util.py:439:  Not all dependencies satisfied
DEBUG util.py:439:  Error: Some packages could not be found.
DEBUG util.py:577:  Child return code was: 1

https://kojipkgs.fedoraproject.org//work/tasks/1857/22891857/root.log
https://koji.fedoraproject.org/koji/taskinfo?taskID=22891830

Comment 8 Lukas Slebodnik 2017-11-03 21:19:30 UTC
(In reply to Lukas Slebodnik from comment #6)
> BTW I am fine with different name for conf_get_* if you want to make them
> public.
> And as you can see we use only "conf_get_str" from private-public interface
> :-)
> And I know we used not really public functions. I am not the author of
> plugin.
> The plugin was not written by sssd core team. It was san external
> contribution.
> 
> But I would like to have as cleanest upgrade path as possible.
> So if you decide to rename, hide some functions then the cleanest solution
> is to bump SONAME. I could workaround it on rpm level even without SONAME
> change but other distributions would have the same problem.
> 
> (In reply to Justin Mitchell from comment #4)
> > Some of these functions could be exposed again, like conf_get_*, but others
> 
> That would be sufficient for sssd plugin and would not break anything.
> 
> > have been changed to fix bugs and to improve thread safety.
> > e.g. the conf_path global has now gone, and been replaced by a parameter to
> > conf_init()
> 
> Do you know about other other plugins which might miss "conf_path" ?
> If know then lets hide them :-)
If "NO" then lets hide tehm :-)

Comment 9 Alice Mitchell 2017-11-06 14:46:09 UTC
(In reply to Lukas Slebodnik from comment #6)
> Do you know about other other plugins which might miss "conf_path" ?
> If know then lets hide them :-)

afaics the only Fedora package that provides a libnfsidmap plugin is sssd-nfs-idmap, searching the wider internet i find one project of libnfsidmap-regex, and little else.

Both of these have bundled a copy of nfsidmap-internal.h that is taken from the libnfsidmap source tree.

For the future the cleanest fix would seem to be to properly publish the parts required to make plugins, perhaps as nfsidmap-plugin.h we can then hide all of the things that shouldn't be there, but for now I will work on a patch that restores the ABI whilst retaining the bugfixes internally.

Comment 10 Steve Dickson 2017-11-06 18:34:02 UTC
Could you please try the rpms under 
   http://people.redhat.com/steved/.bz1509063/

to see if the ABI has been restored.

Comment 11 Lukas Slebodnik 2017-11-06 18:52:25 UTC
(In reply to Steve Dickson from comment #10)
> Could you please try the rpms under 
>    http://people.redhat.com/steved/.bz1509063/
> 
> to see if the ABI has been restored.

I compared with "objdump -T /usr/lib64/libnfsidmap.so.0.3.0"
from libnfsidmap-0.27-3.fc27.x86_64.rpm and all functions are there.

Comment 12 Lukas Slebodnik 2017-11-06 18:57:43 UTC
(In reply to Justin Mitchell from comment #9)
> (In reply to Lukas Slebodnik from comment #6)
> > Do you know about other other plugins which might miss "conf_path" ?
> > If know then lets hide them :-)
> 
> afaics the only Fedora package that provides a libnfsidmap plugin is
> sssd-nfs-idmap, searching the wider internet i find one project of
> libnfsidmap-regex, and little else.
> 

Did you try also check other distributions? debian

> Both of these have bundled a copy of nfsidmap-internal.h that is taken from
> the libnfsidmap source tree.
> 

Thank you for checking it and sorry that sssd team did not try to reach you earlier and ask to make some necessary parts from nfsidmap-internal.h public.

Anyway, thank you very much for cooperation. Because I know there are some users in enterprise https://bugzilla.redhat.com/show_bug.cgi?id=1292238.
And it would be unfortunate to drop this plugin.

Comment 13 Fedora Update System 2017-11-07 19:15:48 UTC
nfs-utils-2.2.1-1.rc1.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-683ce01683

Comment 14 Lukas Slebodnik 2017-11-08 14:10:40 UTC
(In reply to Justin Mitchell from comment #9)
> (In reply to Lukas Slebodnik from comment #6)
> > Do you know about other other plugins which might miss "conf_path" ?
> > If know then lets hide them :-)
> 
> afaics the only Fedora package that provides a libnfsidmap plugin is
> sssd-nfs-idmap, searching the wider internet i find one project of
> libnfsidmap-regex, and little else.
> 
> Both of these have bundled a copy of nfsidmap-internal.h that is taken from
> the libnfsidmap source tree.
> 

What would be the best procedure to make required internal parts public?
Is it enough to file separate fedora BZ and discuss it there?

Comment 15 Alice Mitchell 2017-11-08 14:30:32 UTC
(In reply to Lukas Slebodnik from comment #12)
>> afaics the only Fedora package that provides a libnfsidmap plugin is
>> sssd-nfs-idmap, searching the wider internet i find one project of
>> libnfsidmap-regex, and little else.
>> 
>
> Did you try also check other distributions? debian
Did a quick check of debian stable, the sssd parts they ship dont appear to rely on libndsidmap, so its only nfs-utils that needs it in their case.


(In reply to Lukas Slebodnik from comment #14)
> What would be the best procedure to make required internal parts public?
> Is it enough to file separate fedora BZ and discuss it there?
I'm going to need to look into the political aspects of doing so, but from a coding standpoint i would put most of nfsidmap-internal.h into a new public header file that will go in libnfsidmap-devel, and then lock down all of the symbols that dont absolutely need to be exposed.  but this is best discussed elsewhere now as the fix for this specific bz is on its way.

Comment 16 Steve Dickson 2017-11-08 14:36:51 UTC
(In reply to Justin Mitchell from comment #15)
> (In reply to Lukas Slebodnik from comment #12)
> >> afaics the only Fedora package that provides a libnfsidmap plugin is
> >> sssd-nfs-idmap, searching the wider internet i find one project of
> >> libnfsidmap-regex, and little else.
> >> 
> >
> > Did you try also check other distributions? debian
> Did a quick check of debian stable, the sssd parts they ship dont appear to
> rely on libndsidmap, so its only nfs-utils that needs it in their case.
I'm curious as to why sssd needs the libnfsidmap interface at all?? 

> 
> 
> (In reply to Lukas Slebodnik from comment #14)
> > What would be the best procedure to make required internal parts public?
> > Is it enough to file separate fedora BZ and discuss it there?
> I'm going to need to look into the political aspects of doing so, but from a
> coding standpoint i would put most of nfsidmap-internal.h into a new public
> header file that will go in libnfsidmap-devel, and then lock down all of the
> symbols that dont absolutely need to be exposed.  but this is best discussed
> elsewhere now as the fix for this specific bz is on its way.
Yes, please open another bz to talk about locking down the linfsidmap ABI...

Comment 17 Fedora Update System 2017-11-15 17:47:02 UTC
nfs-utils-2.2.1-1.rc1.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.