[PATCHSET] adding idmap-autorid functionality to net

Michael Adam obnox at samba.org
Mon Sep 30 16:31:56 MDT 2013


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


More information about the samba-technical mailing list