Of challenges in loadparm

[Michael Adam]

Hi Andrew!

Andrew Bartlett wrote:
> 
> I recently had a good chat with Michael Adam <obnox at samba.org> on IRC
> about some of the goals, directions and challenges in Samba's loadparm
> code, and so I wanted to write some of my thoughts down, so we could
> discuss things on the list, and so the discussion can be found by Google
> later :-)

Thanks for writing all this down.

A couple of comments and hints about my current plans below...

> In this mail, I'm speaking particularly about the loadparm code in
> source3/param.  The soruce4/ loadparm code has a similar structure, but
> has solved some of these issues by loosing features. 
> 
> Samba's loadparm has grown over time, and so has a number of features
> that have become embedded into Samba's expected behaviours, but which
> make it harder to implement in a layered fashion.  
> 
> In particular this is a product of the autoconf/makefile object-list
> based build system, because without the restrictions placed by fully
> defined libraries, there is little to prevent the programmer
> implementing layering violations.  There has also been little need to
> force a consistency in implementation, because the original structure
> was so flexible.
> 
> In the past few months, I've been working to put the 'straight-jacket'
> back on loadparm.  I've removed direct assignment of global variables
> (must go via a .special handle routine instead), and ensured that every
> parameter sets a variable in the structure.  I've removed the separate
> variables for the global_myname() and global_myworkgroup(), instead
> storing these directly in loadparm, and using the lp_set_cmdline() code
> to allow overrides where required.  I've also removed the reference to
> lp_string() (and the substitution code it relies on) for a number of
> parameters, and added a way to get at the s3 loadparm in a 'struct
> loadparm_context' expected by the soruce4 code. 
> 
> I've also automated the generation of the struct loadparm_global and
> struct loadparm_service structures.  This has reduced the number of
> places that a parameter is declared, but as Michael pointed out on IRC,
> there is much more to do.  We previously declared a parameter in:
> 
>  - the docs (including the default value)
>  - the parm_struct table (the big C99 table of parameters)
>  - the struct loadparm_service or struct loadparm_globals
>  - the sDefaults and init_globals()
>  - proto.h
> 
> As I mentioned, we now auto-generate struct loadparm_service/struct
> loadparm_globals, which did made it much easier to combine the service
> paramters, but moving from 5 to 4 declarations isn't a solution, it's
> just one small step.  
> 
> As another small step, I would propose that we take the next step (taken
> in source4 for some time) and generate the lp_ prototypes into a
> param_proto.h. 
> 
> We should also rename all the variables in struct loadparm_service and
> struct loadparm_globals to match exactly the lp_ functions that will
> access them, and finally rid us of the 'Hungarian notation' variable
> names.
> 
> However, the correct, long term fix is indeed to generate all these from
> a single table, preferably with some connection to the documentation.
> The challenge here is that we still have very many special cases in our
> loadparm code - a large number of lp_ functions are not in fact
> generated, but are hand-coded to return different values in particular
> circumstances.  Each special case works against useful auto-generation,
> and we should look carefully to see if we can handle them in another
> way. 

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. :-)

> 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. The result will then be processed
in the same way as process_registry_shares() is currently doing.

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

> The other big challenge is the much-used % macros, which are embedded
> strongly as one of the power features of Samba that our most advanced
> (and vocal) users rely on for Samba's full flexibility.  'include = %
> m.smb.conf' is a popular approach (I've certainly used it myself).  This
> similarly violates the parse/load/read layers, because it needs to know
> the machine name for %m.
> 
> One layer that can be split with reasonable ease is the 'get' and 'set'
> layers.  All parameters (even registry parameters) are written into the
> struct loadparm_globals and struct loadparm_service with do_parameter(),
> and the lp_*() functions rely only on the lp_string() and these
> structures.  As such, these could be moved into another file and
> subsystem, potentially breaking some of our last dependency loops. 
> 
> Andrew Bartlett
> -- 
> Andrew Bartlett                                http://samba.org/~abartlet/
> Authentication Developer, Samba Team           http://samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20110721/2663b052/attachment.pgp>