s3-vfs: split @GMT token filter code into a common .c

[Andrew Bartlett]

On Tue, 2012-10-30 at 10:18 +0100, Michael Adam wrote:
> On 2012-10-30 at 19:30 +1100, Andrew Bartlett wrote:
> > On Tue, 2012-10-30 at 08:41 +0100, Volker Lendecke wrote:
> > > On Mon, Oct 29, 2012 at 07:04:38PM +0100, David Disseldorp wrote:
> > > > Hi,
> > > > 
> > > > I've recently been working on a Snapper vfs module[1] to propagate FSRVP
> > > > snapshot create/delete requests to snapperd via D-Bus.
> > > > 
> > > > The module makes use of @GMT token filtering hooks currently provided by
> > > > vfs_shadow_copy2. Therefore, I'd like to split these hooks into a
> > > > common source for use by both modules, similar to the current
> > > > relationship between vfs_acl_tdb.c, vfs_acl_xattr.c and vfs_acl_common.c
> > > > 
> > > > Please see following patch. Feedback appreciated.
> > > 
> > > While it is a good thing to centralize as much of that logic
> > > as possible, please do not #include a .c file. I really,
> > > really do not like it in the acl modules and another use of
> > > that pattern should be avoided if at all possible. If that
> > > means we have to put those routines into core smbd, not
> > > sure.
> > 
> > The challenge here, as I see it, is the autoconf build.  With the
> > infinite flexibility we currently offer on module static/shared we can't
> > get this the sensible way, which is sharing a .c/.o file in the link
> > unit, and the autoconf build doesn't give us easy private shared
> > libraries.
> > 
> > The reason we can't just have both modules link in the extra .c/.o file
> > is that if both are selected static, the symbols would collide in smbd.
> 
> What is the difference, symbol-wise, between two modules that
> include a given .c file and hence create just a single .o file
> each and two modules that link the same additional .o file to
> their own .o files? Is it that the symbols of the shared extra
> .c/.o file would have to be published? If this is the problem,
> I understand that the inclusion might solve the issue when we
> create shared objects for the two modules. But what if we
> link the two .o files directly (and hence) statically into (e.g.) smbd,
> wouldn't the private symbols collide in smbd as well?

If they are 'static' functions in the included .c file, then there won't
be any global symbols to collide.  Otherwise, indeed it would be very
similar.  

One of the biggest frustrations I've faced re-factoring code in Samba is
that the autoconf build has no way to express this dependency or an
automatic way to reduce the object list for uniqueness. 

> I think (and may be mistakten) that the original motivation of
> using the include for the vfs_acl_common.c file was the
> define "ACL_MODULE_NAME" that was done in the vfs_acl_tdb and
> vfs_acl_xattr modules and used in the common file for the module
> parametric conf options. To me this appears simply as lazyness. ;-)
> Jeremy, I apologize in advance if I am not doing you justice here...

Having worked on this code a fair bit recently, I'm rather lost as to
the point of vfs_acl_tdb.  It seems to me that using anything other than
an xattr would create pain - because if an inode is re-used, the old ACL
entry comes back!  The checksum stuff will help of course, but it really
does happen:  In make test, where vfs_xattr_tdb is in use, had to rework
my tests to call via the smbd VFS as inode numbers were being reused
before the script was even finished running!

That is to say, I would love to see this be resolved by removing the
vfs_acl_tdb half of this split module, particularly as we do have
vfs_xattr_tdb for those cases were we don't have xattrs and we are
willing to live with the limitiations.

> To my preference, including .c files is a pattern that we have
> to avoid by all means in samba code. The only exception
> for the rule I would consent to is the case where we want to
> create a standalone unit test program that should test private
> functions of a single given .c file.

We use it in a few places, some for quite some time now.  The test code
you mention is the most prolific, and most creative use of it, but we
also use it in the source4/rpc_server code for the generated
boilerplate, and in the loadparm code.  The loadparm code only used this
as a last resort, due to the need to generate things that would not work
outside that compilation unit. 

I agree we shouldn't use #include "foo.c" where we really just want a
shared library of code.  

> I really have a limited understanding of the linking
> internals/problems here and would like to understand this
> better.

I hope I've clarified things.  

BTW, as my initial mail was in retrospect poorly phrased, it isn't my
intention to open a larger discussion on build systems at this point.
If skipping this module in autoconf allows us to both improve our code
style and share this code neatly, then great, but I'm not advocating any
particular outcome here.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org