[SSSD] third wave for #1747

Simo Sorce simo at redhat.com
Thu Feb 7 21:59:16 UTC 2013


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.

> 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.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list