[SSSD] third wave for #1747

Jakub Hrozek jhrozek at redhat.com
Sun Feb 10 21:07:37 UTC 2013


On Thu, Feb 07, 2013 at 04:59:16PM -0500, Simo Sorce wrote:
> On Tue, 2013-02-05 at 16:45 +0100, Jakub Hrozek wrote:
> 
> Thanks for the review Jakub.
> 
> Replies inline.
> 
> > Patch 0007: Add function get_next_domain()
> > The code looks good, but I would like to fix some code style issues
> > (patch attached, mostly it's one-line statement after for w/o {}
> > brackets, I think it's dangerous and shouldn't be used)
> 
> I included your style changes in my tree.
> 
> > Also can you double check one thing? get_next_domain() supersedes
> > get_next_dom_or_subdom but the order of the check is a little different:
> > 
> > This was get_next_dom_or_subdom():
> >     if (dom->next == NULL && dom->parent != NULL) {
> >         return dom->parent->next;
> >     }
> > 
> >     /* If it's primary domain, the next returned should be its first
> >      * subdomain */
> >     if (dom->subdomains != NULL) {
> >         return dom->subdomains[0];
> >     }
> > 
> >     /* Any other scenario */
> >     return dom->next;
> > 
> > The new get_next_domain() function has the same checks but in a
> > different order:
> >     if (descend && dom->subdomains) {
> >         dom = dom->subdomains;
> >     } else if (dom->next) {
> >         dom = dom->next;
> >     } else if (descend && IS_SUBDOMAIN(dom)) {
> >         dom = dom->parent->next;
> >     } else {
> >         return NULL;
> >     }                      
> > 
> > So with the new code, if "dom" is a primary domain with a sibling, the
> > sibling is returned before descending into the subdomain..that doesn't
> > seem intentional, can you check?
> 
> I do not read them differently.
> 
> As far as I can tell they do the same thing, if there are subdomains,
> they both go through subdomains first, otherwise they jump to the next.
> 
> Keep in mind that here 'next' maybe the next subdomain if dom is a
> subdomain, or the next top level domain if dom is a parent.
> 

Ah, yes, sorry, for some reason I was thinking "top level domain" for
all cases when dom->next is used.

> > Patch 0008: Add ability to disable domains
> > Ack
> > 
> > Patch 0009: Change the way domains are linked
> > Nack. This code crashed for me on every lookup for a trusted user:
> > 
> > +struct sss_domain_info *find_subdomain_by_name(struct sss_domain_info *domain,
> > +                                               const char *name,
> > +                                               bool match_any)
> > +{
> > +    struct sss_domain_info *dom;
> > +
> > 
> > Seems like because dom is uninitialized. I think it should be set to
> > domain, but can you please check that?
> 
> Spot on, sorry about that.
> 
> > +    while (dom && dom->disabled) {
> > +        dom = get_next_domain(dom, false);
> > +    }
> > +    while (dom) {
> > +        if (strcasecmp(dom->name, name) == 0 ||
> > +            ((match_any == true) && (dom->flat_name != NULL) &&
> > +             (strcasecmp(dom->flat_name, name) == 0))) {
> > +            return dom;
> > +        }
> > 
> > Also further in the same function it seems that the code never descends
> > into a subdomain actually as get_next_domain is called with
> > descend=false. Setting it to true made the code work for me, but please
> > check again.
> 
> Ah true!
> I changed both calls to use 'true', thanks for catching this, odd that
> the tests didn't reveal this issue.
> 
> > +        dom = get_next_domain(dom, false);
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > 
> > The patch also didn't apply correctly, but git am -3 worked fine.
> 
> I rebased on master now.

Both trusted and non-trused users seem to be working for me now.



More information about the sssd-devel mailing list