Have patch, need bug-number
The patches to follow depend on the patches for 13802 to be applied
Created attachment 14884 [details] Patch for 4.10
Created attachment 14885 [details] Patch for 4.9
Created attachment 14886 [details] Patch for 4.8
The backports fail the ABI check from --picky-developer: [4087/4217] abi_check: bin/default/source3/libsamba-passdb.so libsamba-passdb.so: symbol xid_to_sid has been added - please mark it _PRIVATE_ or update minor version signature: void (struct dom_sid *, const struct unixid *) libsamba-passdb.so: symbol winbind_xid_to_sid has been added - please mark it _PRIVATE_ or update minor version signature: bool (struct dom_sid *, const struct unixid *) Waf: Leaving directory `/home/cschmit/code/samba/bin' Build failed: ABI for libsamba-passdb.so has changed - please fix library version then build with --abi-update See http://wiki.samba.org/index.php/Waf#ABI_Checking for more information If you have not changed any ABI, and your platform always gives this error, please configure with --abi-check-disable to skip this check make: *** [all] Error 1 For master the ABI signature update is in "lib: Remove some unused code". What is the best way to update the ABI signature for the backports? Just keep the functions in the signature that are removed in master?
I'll find a solution by making that symbol static. I won't have time for this before next week probably, sorry for not checking with due diligence
By the way, we give 100% ABI guarantees within a release. We can NOT change *ANYTHING* user-visible. So we probably need to rewrite the whole patch series to work within the existing semantics.
Adding a symbol isn't usually considered an ABI violation (a minor version update). We already do this don't we ?
Saying "We can NOT change *ANYTHING* user-visible." isn't correct as far as I know. We can (and I think we do) add user-visible symbols so long as we update the minor version number, as existing library users will continue to work.
I think we can sort this out: In master the ABI went from 27.1 to 27.2. This appears wrong now, as functions have been removed, so this should have gone to 28.0. We can simply fix that for master. For the 4.9 backport, that only adds one functions, we create a new signature for the 27.2 ABI. 4.8 only has passdb ABI 27.0. The change from 27.0 to 27.1 only added create_builtin_guests: NTSTATUS (const struct dom_sid *) We could try backporting create_builtin_guests together with the backports to allow the ABI increase to 27.2 for the 4.8 backport. If that sounds correct, i can look into making the additional changes.
Yeah, we should make sure that we push the major number up for our libraries for new Samba releases, that gives up minor version number space to do bugfixes with a forward compatible ABI in a previous version release stream.
I am not sure if we would want to increase the major version for all libraries. Backporting the new functions should usually be feasible. https://lists.samba.org/archive/samba-technical/2019-March/132824.html is an proposed update for the passdb ABI. I will update the backports next.
Created attachment 14897 [details] Patch v2 for 4.8
Created attachment 14898 [details] Patch v2 for 4.9
Created attachment 14899 [details] Patch v2 for 4.10
Can someone explain to me what is expected from me now? I put in the bug, I have to fix it, and I am even paid to fix it. I just don't know what is expected in the form of guarantees that the Samba Team has given to all the downstream users of our passdb public library.
Closing as WONTFIX. Samba has published a fixed API that is severely broken by this kind of change.
Reopening to get feedback from Alexander before we make a final decision.
Christof, can we work with Alexander please to get a fix that works for both Samba and FreeIPA ? Adding Alexander to the CC: list for the bug. Cheers, Jeremy.
(In reply to Jeremy Allison from comment #19) yes, i think Alexander has to comment on what he needs. Maybe not removing the functions in the backports would be sufficient. I also submitted another patch for master that we should include here; third patch in https://lists.samba.org/archive/samba-technical/2019-March/132848.html
(In reply to Volker Lendecke from comment #16) (In reply to Volker Lendecke from comment #17) The pattern we use with ldb was to say that modules (and sssd has ldb modules) must rebuild against the exact version of Samba ldb that they are run with. .so versions are a good thing, but the most important thing they do is assist in communication, and that is the main thing required here. The downstream users here are not far away or uncommunicative, they are part of our community too. Certainly the 'contracts' should be clearer, and reasonable people can disagree as to if it is better to have this in or out of our tree. As you removed it in c5e101af2b16c2869bf5c8f8aa90876aa6450f55 I presume you prefer it out of our tree. I will say it is unsettling to think a job is done and find new requirements, but that again is life in a software development community.
Just to point out: I removed the module because in a side-channel I was told that this version was unused by RedHat and that RedHat develops the FreeIPA module independently of Samba. Do you think that carrying code in Samba that is known to be unused because it is orphaned by the only consumer is the better choice?
(In reply to Volker Lendecke from comment #22) > Just to point out: I removed the module because in a side-channel I was told > that this version was unused by RedHat and that RedHat develops the FreeIPA > module independently of Samba. Do you think that carrying code in Samba that > is known to be unused because it is orphaned by the only consumer is the > better choice? I don't know who told you that but it is clearly not what happens in reality. Especially, given the fact that ipasam is PASSDB module, thus has to be loaded into smbd/winbindd. It cannot be developed independently of Samba. In general, there was no problem in adapting that module to Samba changes over years, as long as there were no attempts to cut off APIs deliberately. 'struct unixid' situation was forced on in 2012 by making sure some of PASSDB APIs were required to pass in 'struct unixid *' pointers. There was no public API to deal with that and we added it, stopped short of actually making gen_ndr/idmap.h a public header to avoid all this problem in the first place. However, my proposal was more humble: do not delete the code in backports you know is used out there. That's all I ask for, especially when a presence of that code is not affecting the code you are backporting.
(In reply to Volker Lendecke from comment #22) No I don't. I think the best place to maintain the module is in FreeIPA's tree, and that any copy we have will just be stale. It is clearly impractical to keep it up to date in our tree, just as we don't carry a stale copy of debian packaging and other things. In this I disagree with Jeremy's mail on the list here: https://lists.samba.org/archive/samba-technical/2019-March/132842.html Where I disagree with you is in suggesting the situation is so impossible that we can't acknowledge and work to resolve user issues concurrent with also being in communion with our friends at FreeIPA. There are practical ways forward, as I wrote. But our users and your fellow developers deserve better than closing the bug as WONTFIX at the first sign of difficultly.
Created attachment 14901 [details] Patch v3 for 4.8.next Includes updates from cs's latest fixes to master. Compiles with --without-winbind also. Christof and Alexander please review.
Created attachment 14902 [details] Patch v3 for 4.8.next Sorry, missed a removed comment in the last patch in the set. This version fixes that.
Created attachment 14903 [details] Patch v3 for 4.9.next
The backports for 4.8 and 4.9 look good to me. Should we keep the removal patch for 4.10 or skip it and only remove the code for master and 4.11?
Please do not backport the removal of unixid_* functions to 4.10. I'll do my review tomorrow by building both samba and freeipa in Fedora COPR.
I'm working on that right now. I think for 4.10.x as Alexander says we need to keep the 'removed' functions (and leave the ABI at 0.27.2) and only remove them for master - 4.11.x (or Samba5, whatever we decide to call it :-).
Created attachment 14904 [details] Patch v3 for 4.10.rcNext Here's the version 3 for 4.10.rcNext - keeps the same ABI as 0.27.2. Includes the changes from master to keep --without-winbind working. With this change we now need to update the ABI number in master to 0.28.0 to match the removed functions.
Alexander, if you can RB+ these once you've tested on Fedora COPR we can get them in for 4.10.rcNext and bugfixes for 4.9.x, 4.8.x. Thanks everyone for your help in reviewing these !
Christof, do you want to propose the ABI update patch for master to 0.28.0, or do you want me to do it ?
(In reply to Jeremy Allison from comment #33) https://lists.samba.org/archive/samba-technical/2019-March/132824.html is the patch on samba-technical to update the ABI number.
Lukas pointed out on that thread that removing a function means a major version bump. Should we go to 1.0 instead ?
(In reply to Jeremy Allison from comment #35) I think this is a wider question. The 0 probably tries to indicate that this is not a stable API. Past changes have used the second number as "major" and the third as "minor" which is not the typical versioning for shared libraries. I think we need to decide: Either stick to the older scheme of increasing the second as "major" and then discuss how to improve versioning for the libraries in Samba for the future. Or make passdb 1.0.0 now and then address the other libraries later. I would like to hear other opinions.
I've built 4.9 branch patches in https://copr.fedorainfracloud.org/coprs/abbra/test-samba-4.9-backport-bug-13813/builds/ and rebuilding freeipa succeeded without additional patches. I had to patch up Fedora 29 libldb and samba to get the patchset applied because v4-9-test requires new libldb (1.4.6 at least) and this one will not be added to Fedora after 4.9.next release. So the patches on 4.9 backport are OK. Since 4.8 is roughly the same, the patchset should be OK. Same for 4.10. I will work on the changes in FreeIPA to adopt to git master asap.
Re-assigning to Karolin for inclusion in 4.10.rcNext, 4.9.next, 4.8.next. Thanks for your help everyone !
(In reply to Jeremy Allison from comment #38) Pushed to autobuild-v4-{10,9,8}-test.
(In reply to Karolin Seeger from comment #39) Pushed to all branches. Closing out bug report. Thanks!
As discussed in a side channel: The fixes break overlapping idmap ranges for rfc2307 "idmap ad" backend.
I would prefer to track this in a new bugzilla: Bug 13903