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(a)redhat.com>
Date: Tue, 28 Jul 2015 13:49:37 +0200
Subject: [PATCH] AD: Use ad_site also when site search fails
ACK