[PATCHSET] adding idmap-autorid functionality to net

Christian Ambach ambi at samba.org
Mon Sep 30 15:52:16 MDT 2013


Am 30.09.13 09:07, schrieb Michael Adam:
> Any comments? Review? :-)

I'll have a look, but it might take a while as this is a large bunch of
changes.
But others can feel free to be quicker than me.

Thanks for cleaning up some stuff that I always wanted to do, but never
found the time for (moving TDB code into seperate file for use by
utility code).

A few initial comments from having an initial look at the patches and
the result after applying all the patches and looking at the complete diff:

* from a quick glance, I would assume that there is some potential for
squashing some of the patches together, especially the ones adding
comments about the functions could be squashed with the ones that add
the implementation of the function, e.g.
2236e54e idmap_autorid: add a comment explaining idmap_autorid_saveconfig()
22677f1 idmap_autorid: add a comment explaining idmap_autorid_loadconfig()
7beabf9 idmap_autorid: add a comment explaining idmap_autorid_db_init()
60d7678 idmap_autorid: add a comment explaining idmap_autorid_init_hwm()
15df1e3 idmap_autorid: add a comment explaining
idmap_autorid_get_domainrange()

* some patches have typos in the messages
some examples
f57e627 atul.kulkarni at in.ibm.com idamp_autorid: factor out domain[...]
416f953 obnox at samba.org.. [...]bahaviour[...]
So please run a double-check on those again

* some formatting issues
sometimes, code format does not always follow README.Coding
e.g.
talloc_free( mem_ctx );

* idmap_autorid_delete_range_by_sid_action/
idmap_autorid_delete_range_by_num_action

Those two are checking for inconsistencies in the mappings, but they are
not able to detect missing forward mappings, only missing backward
mappings. e.g. when sid->id does not exist, but id->sid exists,
delete_range_by_sid action will fail but in theory it could find the
backward mapping with a traversal. The question is if this extra comfort
should be offered or not.

* configuration
idmap_autorid_parse_configstr: why the refactoring of the config
string and the massive code blow-up which instead of passing a simple
line with only mandatory values now does lots of string parsing etc?
Seems a bit over-engineered to me..

Shouldn't idmap_autorid_parse_configstr also check that it found
the minvalue? The idea of this configuration store was to prevent the
administrator from making fatal configuration changes. Now the minvalue
can be changed in the idmapping config and this will mess up all the
mappings.

net idmap set config is a really dangerous command I would say.
You can mess up your complete system when not used appropriately.
Can we add some warning messages here? Especially when somebody tries to
change the rangesize, but mappings already exist? This will end up in a
complete nightmare for the user. IMHO the only parameter that should be
adjustable (while mappings exist) is maxranges, and even that should
only be done after a sanity check.

More to come...

Cheers,
Christian


More information about the samba-technical mailing list