On Fri, Jun 28, 2013 at 08:59:38PM +0200, Sumit Bose wrote:
On Fri, Jun 28, 2013 at 06:43:48PM +0200, Jakub Hrozek wrote:
> Hi,
>
> the attached patches implement
>
https://fedorahosted.org/sssd/ticket/1962. When a new option,
> ipa_server_mode is set to True, then subdomain/trusted users are not
> looked up using the extop plugin but AD ID context is initialized and
> the users are looked up directly with AD code. This is in support of
> legacy clients looking up trusted AD users and groups.
Patches are working well, now trusted domain users and groups can have
algorithmically mapped IDs or IDs from POSIX attributes from the trusted
DC.
[PATCH 1/8] IPA: Add a server mode option
ACK
[PATCH 2/8] LDAP: Add utility function sdap_copy_map
>
> +
> + for (i = 0; i < num_entries; i++) {
> + map[i].opt_name = src_map[i].opt_name;
> + map[i].def_name = src_map[i].def_name;
> + map[i].name = discard_const(src_map[i].def_name);
> + map[i].sys_name = src_map[i].sys_name;
> +
Yes, it would. I have changed the patch. I was just trying to avoid
doing more mallocs than necessary, but actually that already burned me
once with the code as some other part of the back end assumes that the
map is talloc context.
I wonder if it would be safer in the long run to copy the values
instead
of the references. I think currently the options are not modified, but
if a future patch starts to modify values there might be unexpected side
effects?
[PATCH 3/8] AD: decouple ad_id_ctx initialization
ACK
[PATCH 4/8] AD: initialize failover with custom realm, domain and failover service
ACK
I think it would be a good idea if someone is working on some code other
then the initialization routines and sees some calls to configuration
option to evaluate if the call is really needed at this place or if it
would be possible to hand the value over as a parameter. This should
make code more reusable.
Using calls to the samba configuration management was identified as one
of the reasons why some of the samba libraries are hard to use in other
projects. Because it forces the other project to parse a smb.conf file.
I completely agree, the use of confdb_get_* and dp_opt_get_ through the
code was the main reason for this refactoring. But until now the
providers were really self-contained and even the top-level ad_options
or ipa_options variables are provider-global..
But this is something we should think about in the future.. I will add a
note to
https://fedorahosted.org/sssd/ticket/1985 so that this best
practice would be captured in a document somewhere and not forgotten in
a patch review thread.
[PATCH 5/8] IPA: Initialize server mode ctx if server mode is on
ACK
[PATCH 6/8] AD: Move storing sdap_domain for subdomain to genericLDAP code
ACK
[PATCH 7/8] IPA: Create and remove AD id_ctx for subdomains discovered in server mode
ACK
[PATCH 8/8] IPA: Look up AD users directly if IPA server mode is on
ACK
Thank you.
Thank you for the review, new patches are attached.