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 <pc...@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 <pc...@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 <pc...@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 <pc...@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 <pc...@redhat.com> > Date: Mon, 27 Jun 2016 11:53:19 +0200 > Subject: [PATCH 5/5] TESTS: Adding tests for ad_enabled_domains option ACK _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
