[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