[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