[PATCH] Improve lib/param handling and remove another unused script

[Michael Adam]

Hi Garming and Andrew,

Thanks for updating the patches and for your
comments - pushed to master.

Please see comments below for further discussion.

On 2014-01-29 at 19:45 +1300, Andrew Bartlett wrote:
> On Wed, 2014-01-29 at 13:34 +1300, Garming Sam wrote:
> > Hi there,
> > 
> > So I split the two commits and changed the indentation.
> > >
> > > You are adding a check for equality of class
> > > (LOCAL/GLOBAL/SEPARATOR/NONE). It took me a bit to grasp
> > > it: so the offset can be the same for a LOCAL and a GLOBAL
> > > parameter, and if they happen to be neighbors in the param
> > > table, we might illegally turn option A read only
> > > by setting option B on the command line. Nice catch. did
> > > it ever happen?
> > >
> > I think the original reason why they weren't split was because the bug 
> > was actually found with dump_a_parameter coincidentally. It did actually 
> > happen, which was bizarre and luckily Andrew knew what happened. In 
> > dumping a parameter, one parameter somehow lined up with another from 
> > the two structures and so it gave me a strange result. So, we removed 
> > the check for synonyms (which looked at the previous offset) and then we 
> > found it could happen somewhere else.
> > 
> > > But one additional though:
> > > Here (already previously) the direct neighbors of the current parameter
> > > are checked. Is that assumption always safe? In theory this
> > > aliases need not be neighbors in the parm_table, right?
> > > Shouldn't we play it safe and walk the whole table and move
> > > the class and offset checks into the body of the loop?
> > >
> > It seems to be what is expected of the table. If we ever actually 
> > managed to generate the param table, we wouldn't have to worry about it. 
> > It could be made more lenient I guess.
> 
> The reason it doesn't loop over the whole table is to keep the
> complexity at O(n) rather than O(n^2).  With over 300 parameters this is
> important, and as the layout requirement has been strictly observed
> since synonyms were implemented.

Well. I guess this is fair enough.
But I still find this fragile though... :-)

And I don't quite understand your arguments:

- Currently this code is essentially O(1) for each call.
  Looping over all to search for aliases would make it O(n) for each call.
  (What am I missing?)

- And isn't it that the lpcfg_set_cmdline is usually called for
  individual parameters from the command line or in selected
  places? Or is that also called in a loop over all parameters?

- So the impact would not be too bad and essentially only affect
  startup - right?

- One could also add an index or create backlinks by one initial
  traverse if it is too slow.


Scanning the code, I found two more places where similar things
happen.

- One in set_variable which suffers the same local vs
  global problem as lpcfg_set_cmdline.
  --> patch attached.

- One in lpcfg_do_service_parameters() where
  copymapy entries for the parameter and its aliases
  are cleared: This is done by a full walk of the
  list.
  This is performance-wise possibly the worst case.
  because this is afaik called for each parameter
  read from the config each time the config is read.

  Patch for this is following, but I had a small confusion
  in my understanding of the copymap... :-)

  My plan is to follow up with a patch that introduces
  a foreach_alias() function (or so) that takes a callback
  to do the action, so that we can have one central
  implementation of something that is done to or with
  each alias.

> > > But you might explain a bit more in the commit msg:
> > > This was supposed to protect from something in the past
> > > what changed that makes this protection not necessary any more?
> > > Or put differently, do we have parameters starting with "-" now?
> > >
> > I think the major motivation for this change was really just that -valid 
> > was the only thing that was actually hidden. There doesn't seem to be 
> > much reason for hiding it basically internally, with there already a 
> > check for the prefix in other places. When I made the change, I think 
> > Andrew simply told me that the prefix was something that didn't really 
> > take off. Should it be flagged in some other way? If anyone has any 
> > suggestions, feel free to say.
> 
> You are correct, the reason for the change was that it was duplicated
> for the 'dump' case, yet stopped the python bindings and the testparm
> --parameter-name code from reading the value of -valid.  The new docs.py
> test found the special case, because it checked for every documented
> parameter, including -valid. 

There are more occurrences of "-" in the code, e.g. in
lpcfg_next_parameter ..

Cheers - Michael



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-loadparm-fix-offset-bug-in-set_variable-mixing-local.patch
Type: text/x-diff
Size: 1572 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140129/3b79cf46/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 215 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140129/3b79cf46/attachment.pgp>