The Wrapper Project

[Simo]

On Fri, 2013-11-29 at 14:09 +1300, Andrew Bartlett wrote:
> > > As I've said before, in my previous work on this code, I've found and
> > > fixed some very subtle bugs that show up only in bizarre results in our
> > > make test.
> > 
> > Yeah, cause the stuff was simply untested. There were/are no unit tests to 
> > verify that the wrapper functions work as expected. Only nss_wrapper has a 
> > truckload of good tests from Günther. I've added more test and we have a code 
> > coverage of 74.9 %
> > 
> > http://mock.cryptomilk.org/index.php?project=nsswrapper
> > 
> > >  I would want to ensure we had a very strict rule as to
> > > exactly the version of these wrappers that we used against a specific
> > > version of Samba.  I think bundling these like we do popt et al is the
> > > best approach for now.
> > 
> > Why are you (an others) always come up with these lame excuses. Why don't you 
> > just admit that you are lazy and don't want to install any of the 3rdparty 
> > software.
> 
> This isn't about being lazy or otherwise, it is about ensuring that the
> fundamental elements of our test infrastructure remain consistent for
> all test runs.  
> 
> The way I see it is this:  Either the wrapper project is complete, and
> so won't need updates in-tree, or we will need to manage updating the
> build farm, user and developer instances.  Those updates will need to
> strictly (presumably by pkg-config tests strictly checking version
> numbers) match new revisions at exactly the same point where the
> matching change is made in Samba.  

This is easy to do, all these projects are using git, so we can simply
have a file with the git commit you 'standardize on' until the next
wrapper fix. In the buildfarm you pull those git trees, then checkout to
the 'tested' commit hash (probably a version tag anyway) and always
build that specific version of the wrappers, until you need a newer
version due to a fix.

> I'm particularly nervous about this in a 'git bisect' scenario. 

If you *really* care about bisect of samba then you really do not want
to have changing libraries in the tree, because then during the bisect
you *will* have too many moving parts. Not only the samba code you want
to bisect changes, but also the wrapper libraries and many other things.

Actually if you really cared about bisect you should ask for splitting
more stuff out into separate trees, even tests would have to move out,
so you always use the same tests while bisecting ...

> > Last heimdal update: July 2011 (Heimdal last release was done 2012-01-11)
> > Quick copy: 68 files changed, 6750 insertions(+), 3142 deletions(-)
> > 
> > Last zlib update: before 2008 (moved to lib/ in 2008)
> > Last iniparser update: before 2010 (moved to lib/ in 2010)
> > Last popt update: before 2010 (moved to lib/ in 2010)
> 
> I'm not sure what you point is here. 

The point is that we are not good at maintaining external code once you
suck it in the tree.

Instead of embedding the whole code, we should just have a file with a
list of upstream trees (or auto-syncing copies of those trees on
git.samba.org) and then just put a commit hash in the build system. At
least this integrating fixes is only a matter of changing a hash in a
file, and people are not tempted to do one-off fixes in our forked
version and never push it upstream.

For emergency fixes we can commit a patch to apply on top of the checked
out tree.

> > > The other thing I would miss is the ability to invoke this via CPP
> > > macros.  We removed smb_wrapper because we determined the LD_PRELOAD
> > > approach was determined not to be viable in the long term.  I guess the
> > > difference here is that the socket API is more limited, as because CPP
> > > also can't override libc calls, we have altered all the callers anyway.
> > 
> > What is this about now? So I should stop to get 'make test' working with the 
> > LD_PRELOAD wrappers?
> 
> It is my preference that we can use this either way, by CPP or
> LD_PRELOAD. 

The reason why LD_PRELOAD is superior is that it works with all code,
including system libraries, it also allows you to test production
builds. In general we cannot use make test to fully validate production
builds, and that's a really bad thing, it happened before that the
developer build masked bugs that we saw in production instead.

> > > It would, even as he has left Samba development, be insightful to ask
> > > for Tridge's view on this, because he has more experience than anyone
> > > else I know on LD_PRELOAD over many years.
> > 
> > What do you want, that I stop working on this? Why do we dance around the 
> > issue here?
> 
> I wrote positively about this effort when you first spoke of it, and
> raised some issues.  You have returned, with what feels like a 'take it
> or leave it' offer, and without any significant effort to address the
> concerns.  What do you expect me to say?

I do not think many of your concerns are valid, some are but there are
valid alternate ways to solve them. Compromise requires both side to
concede, not just one.

> It is entirely your right to fork a bit of Samba's code, and develop it
> yourself.  However, we should rightly hold it and the changes you
> propose to close scrutiny when you propose to change how Samba uses it. 

For what is worth I think Andreas proposal makes a lot of sense, and I
think most of the objections are more resistance to change than actual
valid showstoppers.

> Getting back to fundamental issues, I'm particularly nervous about your
> dlopen change in ldb, and that it demonstrates some broader challenges
> with the LD_PRELOAD approach:
> 
> https://git.samba.org/?p=asn/samba.git;a=commitdiff;h=c4d1bd522c6850065cd9413ee860856cac52c4df
> 
> At the very least, like we have agreed to do for other areas
> (libwbclient), I think this needs to be set only for build-time
> binaries, so we don't cause issues in the BIND9 server. 
> 
> I hope this helps elaborate on my concerns.  

Linking 2 different Kerberos libraries have other consequences, I know
that this allows you to compile and mask the problem, but it just serves
to only defer issues and make them a lot harder to understand.

For example with latest MIT Kerberos we can use a new ccache type. By
linking Heimdal deep into some library you make understanding/debugging
a misbehaving binary that fails because the deeply embedded/hidden
library does not understand the new ccache type, although most of the
other code handles it just fine.

RTLD_DEEPBIND is just bandaid, the long term solution is to not compile
a private copy of a (subtly or not so subtly) library on systems that
provide one. *Especially* for libraries, but also for client utilities.

Simo.