[PATCHSET] adding idmap-autorid functionality to net

Tue Oct 1 16:57:08 MDT 2013

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/90c27b19/attachment.pgp>