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.