[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