On Thu, Jun 06, 2013 at 01:58:18PM +0200, Sumit Bose wrote:
On Wed, Jun 05, 2013 at 10:06:28PM +0200, Jakub Hrozek wrote:
> On Wed, Jun 05, 2013 at 11:32:39AM +0200, Jakub Hrozek wrote:
> > I pushed the code into my gc branch and I'm attaching an updated tarball
> > again. Sorry for the confusion, I only developed and tested on F19.
>
> Sumit found out that the code crashed after performing a SRV lookup if
> subdomain user was requested. The attached patches implement SRV lookups
> for GC servers properly and protect against cases where no GC servers
> are found at all during service discovery, but later subdomain user is
> requested.
>
> I've pushed the code into my fedorapeople branch and I'm attaching a
> tarball with the latest patches as well.
Thank you, I see no coredump anymore, but still the GC cannot be found
with SRV lookups. But I hve to check my DNS setup as well, so this
might not be a real issue and if, it can be fixed after the beta.
Here are my review comments:
[PATCH 01/15] Do not obfuscate calls with booleans
ACK. I only have one comment. I'm not sure what would be more gdb-friendly,
using defined like in you patch or using real functions for
*_primary_servers_init and _backup_servers_init?
OK, that's a fair comment, I used inline functions in this iteration.
Lukas reminded me that inline functions might not be usable in gdb with
high debug levels, but I think we can live with that.
[PATCH 02/15] LDAP: sdap_id_ctx might contain several connections
ACK
[PATCH 03/15] LDAP: Refactor account info handler into a tevent request
ACK
[PATCH 04/15] LDAP: Pass in a connection to ID functions
ACK
[PATCH 05/15] LDAP: new SDAP domain structure
+int
+sdap_domain_destructor(TALLOC_CTX *ctx)
+{
+ struct sdap_domain *dom =
+ talloc_get_type(ctx, struct sdap_domain);
+ DLIST_REMOVE(*(dom->head), dom);
+ return 0;
+}
destructors use void * as argument.
Fixed.
+void
+sdap_domain_remove(struct sss_domain_info *subdom)
+{
+ struct sss_domain_info *diter;
+
+ if (subdom->parent == NULL) return;
+
+ for (diter = subdom->parent->subdomains;
+ diter;
+ diter = get_next_domain(diter, false)) {
+ if (diter == subdom) break;
+ }
+
+ talloc_free(diter);
+}
I think we should be careful to delete sss_domain_info objects, because they
might be still used by some requests.
That's true, the current patch just removes the domain from the linked
list. I think that removing a domain is such a rare event that for the
time being we can live with the leak. Maybe there could be a ticket to
remove the domain after some period of time or provide a more elaborate
solution, maybe some refcount based on number of requests.
[PATCH 06/15] LDAP: return sdap search return code to ID
ACK
[PATCH 07/15] Move domain_to_basedn outside IPA subtree
ACK
[PATCH 08/15] New utility function sss_get_domain_name
ACK
PATCH 09/15] LDAP: split a function to create search bases
ACK
[PATCH 10/15] LDAP: store FQDNs for trusted users and groups
ACK
[PATCH 11/15] Split generating primary GID for ID mapped users into a separate function
ACK
[PATCH 12/15] LDAP: Do not store separate GID for subdomain users
ACK
PATCH 13/15] AD: Add additional service to support Global Catalog lookups
ACK
[PATCH 14/15] AD ID lookups - choose GC or LDAP as appropriate
+ switch (ar->entry_type & BE_REQ_TYPE_MASK) {
+ case BE_REQ_USER: /* user */
+ case BE_REQ_GROUP: /* group */
+ case BE_REQ_INITGROUPS: /* init groups for user */
+ if (ad_ctx->gc_ctx && IS_SUBDOMAIN(dom)) {
+ clist[i] = ad_ctx->gc_ctx;
+ i++;
+ } else {
+ clist[i] = ad_ctx->ldap_ctx;
+ }
+ break;
+
+ default:
+ clist[0] = ad_ctx->ldap_ctx;
+ break;
+ }
I guess BE_REQ_BY_SECID and BE_REQ_USER_AND_GROUP should be handled like BE_REQ_USER?
Ah, true. The BE_REQ_BY_SECID and BE_REQ_USER_AND_GROUP were developed
in parallel with these patches, so I probably forgot to include them
here.
[PATCH 15/15] AD: Store trusted AD domains as subdomains
typo in the commit message 'newe' -> 'newer'
Fixed.
Thank you for the review. I rebased the patches on top of your PAC
patches (there was one change in pacsrv where I used the new function
sss_get_domain_name) and pushed the patches to my fedorapeople branch
called "gc".
A tarball is also provided for convenience.