[SSSD] third wave for #1747

Jakub Hrozek jhrozek at redhat.com
Tue Feb 5 15:45:04 UTC 2013


On Tue, Jan 15, 2013 at 10:51:30PM -0500, Simo Sorce wrote:
> This is the third, and I think final wave for #1747
> 
> These patches change the way subdomains are handled, eliminates the
> sysdb_subdom structure and changes a number of sysdb_subdomain
> functions.
> 
> The last patch is a fix for #1766 as well.
> 
> Simo.

Patch 0001: Add realm info to sss_domain_info
Ack

Patch 0002: Avoid sysdb_subdom in sysdb_get_subdomains()
Ack

Patch 0003: Update main domain info in place
Ack

Patch 0004: Refactor sysdb_master_domain_add_info()
Ack

Patch 0005: Add sysdb_subdomain_store() function
Ack

Patch 0006: Remove sysdb_subdom completely
Ack

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)

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?

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?

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

+        dom = get_next_domain(dom, false);
+    }
+
+    return NULL;
+}
+

The patch also didn't apply correctly, but git am -3 worked fine.

Patch 0010: Parent and subdomains use the same sysdb
Ack

Patch 0011: Introduce IS_SUBDOMAIN() macro
Ack
-------------- next part --------------
>From 2ef245139fb2e826b0375431e6e3997630554279 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 5 Feb 2013 10:59:38 +0100
Subject: [PATCH] style fix - squash into Add function get_next_domain

---
 src/responder/nss/nsssrv_services.c | 8 ++++++--
 src/tools/sss_debuglevel.c          | 5 ++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c
index ac94ef15c1563fa643959aacff50a917448b7016..7b76cad86e28a7f091833295659836289e351099 100644
--- a/src/responder/nss/nsssrv_services.c
+++ b/src/responder/nss/nsssrv_services.c
@@ -99,8 +99,9 @@ getserv_send(TALLOC_CTX *mem_ctx,
     if (!req) return NULL;
     state->dctx = dctx;
 
-    for (dom = cctx->rctx->domains; dom; dom = get_next_domain(dom, false))
+    for (dom = cctx->rctx->domains; dom; dom = get_next_domain(dom, false)) {
         num_domains++;
+    }
 
     /* Create an array of domains to check. To save resizes, we'll
      * assume that all will be checked
@@ -1258,8 +1259,11 @@ setservent_send(TALLOC_CTX *mem_ctx, struct cli_ctx *cctx)
      * to reallocate later
      */
     num_domains = 0;
-    for (dom = state->cctx->rctx->domains; dom; dom = get_next_domain(dom, false))
+    for (dom = state->cctx->rctx->domains;
+         dom;
+         dom = get_next_domain(dom, false)) {
         num_domains++;
+    }
 
     state->nctx->svcctx->doms = talloc_zero_array(state->nctx->svcctx,
                                                   struct dom_ctx,
diff --git a/src/tools/sss_debuglevel.c b/src/tools/sss_debuglevel.c
index a24bfef58a65c680c76f43695207f772aa9ed027..ce428bd9209f46fb2dd0d926bd2cab8decd6c2be 100644
--- a/src/tools/sss_debuglevel.c
+++ b/src/tools/sss_debuglevel.c
@@ -253,8 +253,11 @@ errno_t get_confdb_sections(TALLOC_CTX *ctx, struct confdb_ctx *confdb,
     if (ret != EOK)
         DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to get domain list\n"));
 
-    for (domain = domain_list; domain; domain = get_next_domain(domain, false))
+    for (domain = domain_list;
+         domain;
+         domain = get_next_domain(domain, false)) {
         domain_count++;
+    }
 
     /* allocate output space */
     sections = talloc_array(ctx, char*,
-- 
1.8.1



More information about the sssd-devel mailing list