[SSSD] [PATCH] AD: Use ad_site also when site search fails

Jakub Hrozek jhrozek at redhat.com
Wed Jul 29 16:43:29 UTC 2015


On Wed, Jul 29, 2015 at 02:30:15PM +0200, Jakub Hrozek wrote:
> On Wed, Jul 29, 2015 at 11:07:09AM +0200, Pavel Březina wrote:
> > On 07/28/2015 08:55 PM, Jakub Hrozek wrote:
> > >On Tue, Jul 28, 2015 at 08:51:22PM +0200, Jakub Hrozek wrote:
> > >>On Tue, Jul 28, 2015 at 01:51:13PM +0200, Pavel Březina wrote:
> > >>>On 07/28/2015 01:42 PM, Jakub Hrozek wrote:
> > >>>>On Tue, Jul 28, 2015 at 12:13:31PM +0200, Pavel Březina wrote:
> > >>>>>On 07/27/2015 09:35 PM, Jakub Hrozek wrote:
> > >>>>>>On Mon, Jul 27, 2015 at 07:38:12PM +0200, Pavel Březina wrote:
> > >>>>>>>On 07/27/2015 05:38 PM, Jakub Hrozek wrote:
> > >>>>>>>>On Mon, Jul 27, 2015 at 12:32:59PM +0200, Pavel Březina wrote:
> > >>>>>>>>>On 07/21/2015 09:45 PM, Jakub Hrozek wrote:
> > >>>>>>>>>>Hi,
> > >>>>>>>>>>
> > >>>>>>>>>>the attached patch helps AD clients that have trouble detecting the
> > >>>>>>>>>>right site. Please see the patch and the commit message for more
> > >>>>>>>>>>details.
> > >>>>>>>>>
> > >>>>>>>>>Hmm... nack. I think this change makes better job:
> > >>>>>>>>>
> > >>>>>>>>>     if (state->ctx->ad_site_override != NULL) {
> > >>>>>>>>>         DEBUG(SSSDBG_TRACE_INTERNAL,
> > >>>>>>>>>               "Ignoring AD site found by DNS discovery: '%s', "
> > >>>>>>>>>               "using configured value: '%s' instead.\n",
> > >>>>>>>>>               state->site, state->ctx->ad_site_override);
> > >>>>>>>>>         state->site = state->ctx->ad_site_override;
> > >>>>>>>>>+       ret = EOK;
> > >>>>>>>>>     }
> > >>>>>>>>
> > >>>>>>>>That was my first version, too, but it appears that the EOK branch tries
> > >>>>>>>>to access state->forest that is not known -- keep in mind that this is
> > >>>>>>>>for cases where ad_get_client_site_recv() returned ENOENT so its output
> > >>>>>>>>parameters might not be available.
> > >>>>>>>>
> > >>>>>>>>Can we infer the forest somehow?
> > >>>>>>>
> > >>>>>>>Is forest not known or is it just not set because _recv returned error?
> > >>>>>>
> > >>>>>>I don't think we can make any assumptions unless we change the _recv to
> > >>>>>>return a special error code for the case where the request was only able
> > >>>>>>to read the forest. But looking at the request, both are read at the
> > >>>>>>same time using ad_get_client_site_parse_ndr().
> > >>>>>>
> > >>>>>>>The
> > >>>>>>>latter one can be solved... but it may be even more conditions :-)
> > >>>>>>>
> > >>>>>>>OK then.
> > >>>>>>
> > >>>>>>Can you review the patch, please?
> > >>>>>
> > >>>>>How about this:
> > >>>>>
> > >>>>>diff --git a/src/providers/ad/ad_srv.c b/src/providers/ad/ad_srv.c
> > >>>>>index f3a4e64..dabcf9f 100644
> > >>>>>--- a/src/providers/ad/ad_srv.c
> > >>>>>+++ b/src/providers/ad/ad_srv.c
> > >>>>>@@ -774,6 +774,12 @@ static void ad_srv_plugin_site_done(struct tevent_req
> > >>>>>*subreq)
> > >>>>>                "using configured value: '%s' instead.\n",
> > >>>>>                state->site, state->ctx->ad_site_override);
> > >>>>>          state->site = state->ctx->ad_site_override;
> > >>>>>+
> > >>>>>+        if (state->forest == NULL) {
> > >>>>>+            state->forest = state->discovery_domain;
> > >>>>>+        }
> > >>>>>+
> > >>>>>+        ret = EOK;
> > >>>>>      }
> > >>>>>
> > >>>>>With some debug message probably.
> > >>>>
> > >>>>Yes, that's better, do you want to send this as a git-formatted patch?
> > >>>
> > >>>Attached.
> > >>
> > >>ACK
> > >
> > >Actually sorry, I was being careless in the review. The code works, but
> > >there is a warning:
> > >
> > >/home/remote/jhrozek/devel/sssd/src/providers/ad/ad_srv.c: In function
> > >'ad_srv_plugin_site_done':
> > >/home/remote/jhrozek/devel/sssd/src/providers/ad/ad_srv.c:781:27:
> > >warning: assignment discards 'const' qualifier from pointer target type
> > >[-Wdiscarded-qualifiers]
> > >              state->forest = state->discovery_domain;
> > >                                         ^
> > 
> > Ah, sorry about that. I made state->forest const as that is what we did with
> > state->site recently. So we are consistent there.
> > 
> 
> > From 4cd7ffe4d4275889ac5dfaf68f4608e515fab0d5 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Tue, 28 Jul 2015 13:49:37 +0200
> > Subject: [PATCH] AD: Use ad_site also when site search fails
> 
> ACK

* master: cbbd8ce524a7e1ae0a1b553c2af18fbef59a06ce



More information about the sssd-devel mailing list