[PATCHSET] adding idmap-autorid functionality to net

[Michael Adam]

I updated my branch incorporating the fixes below
and a few fixes / improvements based on comments by Atul.

http://gitweb.samba.org/?p=obnox/samba/samba-obnox.git;a=shortlog;h=refs/heads/master-idmap-autorid

If desired, I can also post the updated patchset here.

Thanks - Michael

On 2013-10-01 at 00:31 +0200, Michael Adam wrote:
> Hi Christian,
> 
> Thanks for your initial feedback.
> 
> On 2013-09-30 at 23:52 +0200, Christian Ambach wrote:
> > 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()
> 
> Well, these functions were not added, but moved from
> idmap_autorid.c to new idmap_autorid_tdb.c.
> So I left that commenting separate from moving since I think
> different things belong to different patches.
> 
> > * some patches have typos in the messages
> > some examples
> > f57e627 atul.kulkarni at in.ibm.com idamp_autorid: factor out domain[...]
> > will fix.
> 
> > 416f953 obnox at samba.org.. [...]bahaviour[...]
> 
> I don't find that patch.  I found one patch
> "idmap_autorid: remove fstring keystr from autorid_range_config"
> that spells "behavior" which is correct american spelling.
> But I agree that we should stick to british spelling... ;)
> 
> > 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 );
> 
> Oh, I missed that one, will fix it.
> 
> > * 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..
> 
> Same here. I personally was ok with the original scanf
> implementation, but Atul made a very strong point of wanting
> to be more generous to config strings passed in on the command
> line. Plus it can print more precise error messages. So I did
> not object too much. ;-) But based on your comment, maybe I
> should rethink...
> 
> > 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.
> 
> Hm, I don't see that the patch introduces a regression to the
> older code. idmap_autorid_parse_configstr() just parses a config
> string from whatever source. It will fail if no minvalue is
> provided.
> 
> The fn idmap_autorid_saveconfig() compares the handed in config
> with the stored one and forbids the storing when the minvalue
> or the rangesize would change, just as before. This check
> was originally in idmap_autorid_initialize() and was moved
> to saveconfig to allow for implementation "net idmap set config".
> 
> > 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?
> 
> As I wrote above, this is not possible.
> 
> > This will end up in a complete nightmare for the user. IMHO the
> > only parameter that should be adjustable (while mappings exist)
> > is maxranges,
> 
> This is the case.
> 
> > and even that should only be done after a sanity check.
> 
> What kind of check?
> We do check that the hwm value is still <= maxranges, so as not
> to effectively filter out existing range mappings.
> 
> Generally, all the writing commands for the autorid db are
> dangerous of course (as are commands for writing any idmap
> database), and warnings are appropriate. Possibly at least
> in the manpage (patch pending).
> 
> I am going to send an updated patchset including some fixes
> most probably tomorrow.
> 
> Cheers - Michael


-------------- 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/20131001/437b7774/attachment.pgp>