On Mon, Jul 25, 2016 at 06:18:28PM +0200, Petr Cech wrote:
Hello,
there is fixed patch set attached.
Segmentation fault was caused by wrong pointer :-(, sorry.
This new patch set has new debug message. I am open to dissccus the
debug_level and content of message. Any improving idea?
I hit one issue during testing -- sometimes if I am connected to subdomain
and I enable only sibling subdomain (the master is added automaticaly) and
forest root is not enabled -- I see only master and sibling not. But if I
added sleep for cycle (for using dbg) to function ad_subdomains_init()
everythink is OK.
Any idea?
Can you test that case with valgrind? This sounds like some uninitilized
variable condition.
Anyway, I would like to ask you for testing.
Regards
--
Petr^4 Čech
From 7fd9ad8f42f274463c1d107b195d21290fd0b0f2 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Fri, 13 May 2016 05:21:07 -0400
Subject: [PATCH 1/5] AD_PROVIDER: Add ad_enabled_domains option
ACK
From dfa6e063d3ef4850a7809e2f5a6d3826ea061b27 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Tue, 21 Jun 2016 08:34:15 +0200
Subject: [PATCH 2/5] AD_PROVIDER: Initializing of ad_enabled_domains
We add ad_enabled_domains into ad_subdomains_ctx.
Resolves:
https://fedorahosted.org/sssd/ticket/2828
---
src/providers/ad/ad_subdomains.c | 82 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
index e9da04e384e598927f9c8c203a751bcccd29e895..9c0afb19418e44a3e3daa661baf1c7a82439d60d
100644
--- a/src/providers/ad/ad_subdomains.c
+++ b/src/providers/ad/ad_subdomains.c
@@ -57,6 +57,79 @@
/* do not refresh more often than every 5 seconds for now */
#define AD_SUBDOMAIN_REFRESH_LIMIT 5
+static errno_t ad_get_enabled_domains(TALLOC_CTX *mem_ctx,
+ struct ad_id_ctx *ad_id_ctx,
+ const char *ad_domain,
+ const char ***_ad_enabled_domains)
+{
+ int ret;
+ const char *str;
+ const char *option_name;
+ const char **domains = NULL;
+ int count;
+ bool is_ad_in_domains;
+ TALLOC_CTX *tmp_ctx = NULL;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
+
+ str = dp_opt_get_cstring(ad_id_ctx->ad_options->basic, AD_ENABLED_DOMAINS);
+ if (str == NULL) {
+ _ad_enabled_domains = NULL;
I think you wanted to dereference the pointer here? (it might also be a
good idea to return EINVAL if the caller passed NULL as the output pointer)
+ ret = EOK;
+ goto done;
+ }
+
From 4cd5c77f09750f9eb20c91eadc4759c3e4166093 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Tue, 21 Jun 2016 09:48:52 +0200
Subject: [PATCH 3/5] AD_PROVIDER: ad_enabled_domains - only master
We can skip looking up other domains if option ad_enabled_domains
contains only master domain.
Resolves:
https://fedorahosted.org/sssd/ticket/2828
---
src/providers/ad/ad_subdomains.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
index 9c0afb19418e44a3e3daa661baf1c7a82439d60d..eaa85f802dbb4022dc632cc4c05d89685eccd901
100644
--- a/src/providers/ad/ad_subdomains.c
+++ b/src/providers/ad/ad_subdomains.c
@@ -1163,6 +1163,7 @@ static void ad_subdomains_refresh_connect_done(struct tevent_req
*subreq)
return;
}
+ /* connect to the DC we are a member of */
subreq = ad_master_domain_send(state, state->ev, state->id_ctx->conn,
state->sdap_op,
state->sd_ctx->domain_name);
if (subreq == NULL) {
@@ -1211,6 +1212,21 @@ static void ad_subdomains_refresh_master_done(struct tevent_req
*subreq)
goto done;
}
+ /*
+ * If ad_enabled_domains contains only master domain
+ * we shouldn't lookup other domains.
+ */
+ if (state->sd_ctx->ad_enabled_domains != NULL) {
+ if (talloc_array_length(state->sd_ctx->ad_enabled_domains) == 2) {
+ if (strcmp(state->sd_ctx->ad_enabled_domains[0],
+ state->be_ctx->domain->name) == 0) {
I only notices this now, but IIRC the domains are case insensitive,
shouldn't we use strcasecmp here?
+ DEBUG(SSSDBG_TRACE_FUNC,
+ "No other enabled domain than master.\n");
+ goto done;
+ }
+ }
+ }
+
subreq = ad_get_root_domain_send(state, state->ev, forest,
sdap_id_op_handle(state->sdap_op),
state->sd_ctx);
--
2.7.4
From 0d2b0c67875663d0e614b8361152ec7edc14c37e Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Mon, 27 Jun 2016 11:51:30 +0200
Subject: [PATCH 4/5] AD_PROVIDER: ad_enabled_domains - other then master
We can skip looking up other domains if
option ad_enabled_domains doesn't contain them.
Resolves:
https://fedorahosted.org/sssd/ticket/2828
---
src/providers/ad/ad_subdomains.c | 49 ++++++++++++++++++++++++++++++++++++----
1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
index eaa85f802dbb4022dc632cc4c05d89685eccd901..32f7d7b3b2c4917d15d516c5e8beac159ffe164f
100644
--- a/src/providers/ad/ad_subdomains.c
+++ b/src/providers/ad/ad_subdomains.c
@@ -130,6 +130,23 @@ done:
return ret;
}
+static bool is_domain_enabled (const char *domain,
+ const char **enabled_doms)
^
We don't put a space between function name and the argument list.
+{
+ bool ret;
+
+ if (enabled_doms == NULL) {
+ ret = true;
+ goto done;
Do we really need to use jump labels instead of just returning true?
+ }
+
+ ret = string_in_list(domain, discard_const_p(char *, enabled_doms), true) ?
+ true : false;
Can you also fix the indentation here? We could also just return the
function return value and get rid of the 'ret' variable completely.
+
+done:
+ return ret;
+}
+
static errno_t
ad_subdom_ad_ctx_new(struct be_ctx *be_ctx,
struct ad_id_ctx *id_ctx,
@@ -485,6 +502,7 @@ done:
From 92af52a3b4d8fa138282030a72122d6cbe9c5413 Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Mon, 27 Jun 2016 11:53:19 +0200
Subject: [PATCH 5/5] TESTS: Adding tests for ad_enabled_domains option
ACK