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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel