[PATCH] Improve lib/param handling and remove another unused script
abartlet at samba.org
Tue Jan 28 23:45:57 MST 2014
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.
> > 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.
Thanks for updating the patches!
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
More information about the samba-technical