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

Jakub Hrozek jhrozek at redhat.com
Tue Jul 28 18:51:22 UTC 2015


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

> I'm sorry for taking over your contribution :-)
> 

Well, your code is simply better :-) thanks for proposing it.



More information about the sssd-devel mailing list