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

Pavel Březina pbrezina at redhat.com
Wed Jul 29 09:07:09 UTC 2015


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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AD-Use-ad_site-also-when-site-search-fails.patch
Type: text/x-patch
Size: 2181 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150729/556b7827/attachment.bin>


More information about the sssd-devel mailing list