On Fri, Jun 07, 2013 at 12:27:49AM +0200, Jakub Hrozek wrote: > On Thu, Jun 06, 2013 at 08:45:34PM +0200, Sumit Bose wrote: > > On Thu, Jun 06, 2013 at 08:06:22PM +0200, Jakub Hrozek wrote: > > > 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. > > > > ACK, please do not forget to open a ticket about the leaking domain. > > > > bye, > > Sumit > > Thank you for the review, all patches were pushed to master. > > I didn't push your additional patch to amend the service discovery to > use the forest value instead of the domain. Can you please open a ticket > to fix lookups when enrolled with a different server than the forest > root? I think that given you already had a partial patch you know the > details better.
sure, it's https://fedorahosted.org/sssd/ticket/1973 . bye, Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel