In s3compat, to ifdef or not to ifdef, that's the issue

[Andrew Bartlett]

On Tue, 2010-05-25 at 20:37 -0400, simo wrote:
> While reviewing abartlet's merge branch for the s3compat effort I came
> up on a change in loadparm.c that splits out some functions in a
> separate file. The intent is to not build that file in the merged build
> because the s4 side of the code provides it's own version.
> 
> In this case to me it seemed a better choice to just use an #ifdef. The
> reason is that loadparm.c traditionally has been a single file and
> people expect to find everything lp_* related in there. But also that
> this makes it easy to see in the code what pieces still causes conflicts
> (just search for the ifdefs), while a separate file doesn't give much
> clue unless you go and compare which files are included in Makefile.in
> vs what are included in wscript files.

I agree, it's a difficult issue.  I defence of the current approach, I
will note:
 - most of the files that I split up were very, very long to begin with.
As s3compat tends to replace related functions, the new files are for
the most part a natural grouping of functions, and an otherwise useful
cleanup. 
 - That lp_ function are already defined outside loadparm -
lp_workgroup() is defined in lib/util.c (now in lib/util_names.c). 
 - I wasn't confident that s3compat would be accepted in the long term,
and didn't want to leave an #ifdef spegetti all over the code.
 - I've found that the same problems s3compat has hit are also
experienced by the vfstest code, which duplicated functions from
server.c rather than attempt a re-factor.  I've been able to remove that
duplication. 

> This is my take, but I guess that ifdefs may also be annoying in some
> cases.
> So I think it would be nice to have input from those who care about what
> is the approach people like more.
> 
> a) #ifdef blocks around functions
> b) move functions out of the way in separate files
> 
> I think in some cases it may also depend on the subsystem, so comments
> are very welcome.

It would certainly be much easier for me to just #ifdef out the
functions that I need to replace.  In terms of build systems, it would
also be easier if I can reuse the samba3 waf rules with just -D and -I
manipulations.  (ie, add #ifndef ENABLE_S3COMPAT to the files I've split
out). 

If that is acceptable to the rest of the team, I would like to use that
approach for future changes.  For example, it would be awkward to try
implement the attached 2 patches any other way.

Andrew Bartlett

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: commit-s3compat-lp_passdb-lp_security.patch
Type: text/x-patch
Size: 1603 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100526/5cec3818/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: commit-sec-ads-s3compat.patch
Type: text/x-patch
Size: 807 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100526/5cec3818/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100526/5cec3818/attachment.pgp>