Of challenges in loadparm

[Andrew Bartlett]

On Thu, 2011-07-21 at 13:49 +0200, Michael Adam wrote:

> Indeed, the ultimate goal should be to have one big input for all
> the above structures, that should generate the service and global
> structs including the defaults, the parm tables and the doc
> snippets for the parameter descriptions. This could (as Metze had
> suggested to me) be realized as one big python or perl table
> which is traveresed by the genrerating script to extract the bits
> to generate the code and doc files.
> 
> But as you have written above, there are many details that need
> to be solved first.
> 
> While I do of course appreciate that you went the step to generate
> at least a part of the data, I would like to mention my concern that
> I have with the solution:
> 
> * It parses loadparm.c for the occurrences of ^FN_... to
>   determine the member variables in the global and service
>   structs. This generates the global and service header files
>   defining these structs. They use macros to include the
>   remaining special variables that can't be autogenerated
>   from loadparm.c. These macros are defined in loadparm.c
>   the generated header files are then included in loadparm.c.
> 
> * This circle
>    parse loadparm.c
>     --> generate headers
>      --> include headers in loadparm.c
>       --> use defines from loadparm.c in the headers
>   somehow leaves me with a bad feeling.
> 
>   I also (very personally) dont like that it stems on the mkproto
>   mechanism and in particular uses leading spaces before "FN_..."
>   as a singal to skip the prototype adds an element of
>   arbitrariness for me.
> 
> This roundabout fashion is why I consider this as an intermediate
> band-aid and I would like to replace with some autogeneration from an
> external soucrce soon. As I said in the chat, I will think about
> this and hope that I will find the time to work on something like
> this soon. :-)

I guess I don't really see why the 'loop' here is an issue, given the
separate build stages involved.  As to the significance of leading
spaces, aside from being a convention that has worked quite well for
Samba for quite some time, I don't mind if it's changed to a CPP token
that is defined away. 

I would like to see this stage extended to restore automatic prototype
generation of loadparm functions, in the interim before a more grand
scheme can be achieved.  Doing so would make changes in this area much
less error prone (I'm pretty sure there are both missing and extra
prototypes at the moment). 

> > We also discussed the idea of introducing much more layering to the
> > loadparm code, creating a distinct 'get' and 'set' layer, and possible
> > first parsing the smb.conf, then implementing it. 
> > 
> > The history of loadparm presents a number of challenges in the
> > introduction of layering, because a number of parameters impact on the
> > loading of the smb.conf file, or the display of error messages from that
> > load process.  So, for example:
> >                 DEBUG(3, ("Processing section \"[%s]\"\n",
> > pszSectionName));
> > This is shown at a debug level above the default, so only appears if the
> > debug is set on the command line, or earlier in the smb.conf.  This
> > works because the .special handler for 'log level' causes this to be
> > acted on during the parse.  
> > 
> > Similarly, the 'config file' and 'include = ' directives are meta
> > directives, which need to be acted on as part of the parse - the
> > 'include = registry' in particular introducing the full complexity of
> > the registry TDB load to the parsing.  (This is also a source of
> > dependency issues).
> 
> I just wanted to let you know that I am planning to add something
> to s3 loadparm soon. I have added something called libsmbconf
> some time ago to support registry configuration in samba3
> loadparm. And it is already used for registry config there.
> See process_registry_shares() and process_registry_service().
> This already implements the layer separation of parsing and
> implementing/activating the changes. The libsmbconf also has a
> text backend that uses the param.c parser from loadparm. I will
> add a "flat" backend to libsmbconf that will parse the whole tree
> of includes, keeping track of config files/sources. The resulting
> array of smbconf sections will ideally not have includes any more but
> will be completely expanded. 

Have you seen the code Jelmer did for generic parameter handling?  It's
not very much, nor it is much used, but it's in lib/util/paramlist.c and
lib/util/paramlist.h and you might want to take a look at it. 

> The result will then be processed
> in the same way as process_registry_shares() is currently doing.

The big issue I see with this is the immediate application of loadparm
variables.  But given we have ways of setting the log level on the
command line, removing the immediate application would certainly make
this much less delicate area of code, if we think we can make that
change.  

> As a start, also working towards a unification of the code,
> I am adding a loadparm context to s3 loadparm.c to carry all
> those nasty global variables.
> 
> Cheers - Michael

If you do move towards a context here, if it could be the same one
already used in Samba4 it would be very helpful.  In particular, being
able to pass it to the lpcfg_ functions would be very useful.  That is,
a way to split the lpcfg_ functions off from the rest of the source4
loadparm, and get a context from a function like loadparm_init_s3() that
similarly has few deps would be really useful.   (The best course of
action here might actually be to split these parts of s4's loadparm into
some common code). 

Whatever we do here, I would ask that you either make the same change to
the s4 loadparm, or do more work to integrate the two systems before you
diverge them again.  The two systems are getting much closer, and it
would be unfortunate to diverge them again before we get a single
system. 

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