[PATCHSET] adding idmap-autorid functionality to net

[Michael Adam]

Pushed to autobuild after last review by Volker.

Thanks - Michael

On 2013-10-02 at 00:57 +0200, Michael Adam wrote:
> FYI: Volker has been reviewing the patchset thoroughly
> today and we did a few improvements and fixes.
> We removed the parse_configstr bloatup.
> 
> Latest state in my master-idmap-autorid-review-vl
> and master-idmap-autorid-review-vl-squashed branches.
> (First branch contains 3 small improvements to be squashed
> after review which has already been performed on the
> second branch.)
> 
> Will most probably push tomorrow morning after a final look.
> 
> Cheers - Michael
> 
> On 2013-10-01 at 10:54 +0200, Michael Adam wrote:
> > 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/20131002/3e3a8656/attachment.pgp>