[PATCH] Improve lib/param handling and remove another unused script
Andrew Bartlett
abartlet at samba.org
Wed Jan 29 11:57:20 MST 2014
On Wed, 2014-01-29 at 13:34 +0100, Michael Adam wrote:
> 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?)
That looking for the parameter in the first place is O(n), so we get to
O(n^2).
> - 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.
I think we should move to better data structures in the medium term.
> 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... :-)
Garming and I found many - too many - things in loadparm that are either
just a little too subtle for their own good, or just plain confusing.
Some from the code split, some from the kludges I did for the merge, and
much from 20 years of history.
The good news is that by working in the code and making it ready for
more auto-generation, we are already flushing out many strange things
that don't need to be true any more (like magic behaviour for -),
finding other special cases that don't need to be special any more, and
generally making things more consistent. We think we can remove much of
the complexity from the loadparm_s3_context case!
> 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.
Sounds good. That is indeed an important O(n^2) case.
> > > > 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 ..
Sure, and there is may have made sense, as lpcfg_next_parameter is like
dump, an enumeration case. Happily it is also unused, attached is the
patch to just remove it, along with the also-unused lp_is_default.
Please review/push.
Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lib-param-Remove-unused-lpcfg_next_parameter-once-us.patch
Type: text/x-patch
Size: 2647 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140130/0c8ff555/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-param-Remove-unused-lp_next_parameter-once-used-by-S.patch
Type: text/x-patch
Size: 3043 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140130/0c8ff555/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-param-Remove-unused-lp_is_default-once-used-by-SWAT.patch
Type: text/x-patch
Size: 1928 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140130/0c8ff555/attachment-0002.bin>
More information about the samba-technical
mailing list