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.
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
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
(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.
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()
(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...
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 :-)
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
(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 :-)
(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.
Could you please try the rpms under http://people.redhat.com/steved/.bz1509063/ to see if the ABI has been restored.
(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.
(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.
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
(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?
(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.
(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...
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.