On Thu, Sep 19, 2013 at 10:47:49AM +0200, Pavel Březina wrote:
> On 09/18/2013 07:26 PM, Jakub Hrozek wrote:
>> On Wed, Sep 18, 2013 at 04:42:38PM +0200, Pavel Březina wrote:
>>> On 09/17/2013 10:18 PM, Jakub Hrozek wrote:
>>>> On Fri, Sep 13, 2013 at 01:04:28PM +0200, Pavel Březina wrote:
>>>>> On 09/04/2013 08:16 AM, Jakub Hrozek wrote:
>>>>>> This patch depends on my other AD enumeration patches.
>>>>>>
>>>>>>
https://fedorahosted.org/sssd/ticket/2067
>>>>>>
>>>>>> Some AD or AD-like servers do not contain the netlogon attribute
in the
>>>>>> master domain name. Instead of failing completely, we should just
abort
>>>>>> the master domain request and carry on. The only functionality we
miss
>>>>>> would get getting users by domain flat name.
>>>>>
>>>>> Nack (minor)
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> thank you for the review.
>>>>
>>>>>> AD: Failure to get flat name is not fatal
>>>>>>
>>>>>>
https://fedorahosted.org/sssd/ticket/2067
>>>>>>
>>>>>> Some AD or AD-like servers do not contain the netlogon
attribute in the
>>>>>> master domain name. Instead of failing completely, we should
just abort
>>>>>> the master domain request and carry on. The only
functionality we miss
>>>>>> would get getting users by domain flat name.
>>>>> ^ be?
>>>>>
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>>> static void
>>>>>> ad_master_domain_netlogon_done(struct tevent_req *subreq)
>>>>>> {
>>>>>> int ret;
>>>>>> size_t reply_count;
>>>>>> struct sysdb_attrs **reply = NULL;
>>>>>>
>>>>>> struct tevent_req *req = tevent_req_callback_data(subreq,
>>>>>> struct
tevent_req);
>>>>>> struct ad_master_domain_state *state =
>>>>>> tevent_req_data(req, struct ad_master_domain_state);
>>>>>>
>>>>>> ret = sdap_get_generic_recv(subreq, state, &reply_count,
&reply);
>>>>>> talloc_zfree(subreq);
>>>>>> if (ret != EOK) {
>>>>>> DEBUG(SSSDBG_OP_FAILURE, ("sdap_get_generic_send
request failed.\n"));
>>>>>> tevent_req_error(req, ret);
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> if (reply_count != 1) {
>>>>>> if (reply_count == 0) {
>>>>>> DEBUG(SSSDBG_MINOR_FAILURE, ("No netlogon data
available. Flat name " \
>>>>>> "might not be
usable\n"));
>>>>>> } else if (reply_count > 1) {
>>>>>> DEBUG(SSSDBG_MINOR_FAILURE,
>>>>>> ("More than one netlogon info
returned.\n"));
>>>>>> }
>>>>>> /* Not fatal. Just quit. */
>>>>>> goto done;
>>>>>> }
>>>>>
>>>>> Removing != 1 condition will make the code nicer.
>>>>
>>>> OK, I removed the condition.
>>>>
>>>>>
>>>>>>
>>>>>> ret = netlogon_get_flat_name(state, reply[0],
&state->flat);
>>>>>> if (ret != EOK) {
>>>>>> DEBUG(SSSDBG_MINOR_FAILURE, ("Could not get the flat
name\n"));
>>>>>> /* Not fatal. Just quit. */
>>>>>> goto done;
>>>>>> }
>>>>>>
>>>>>> DEBUG(SSSDBG_TRACE_FUNC, ("Found flat name
[%s].\n", state->flat));
>>>>>> done:
>>>>>> tevent_req_done(req);
>>>>>> return;
>>>>>> }
>>>>
>>>> New patches are attached.
>>>
>>> Hi,
>>> sorry, I should have nitpick this earlier. When you fail to obtain a
>>> flat name, logs contains:
>>> [ad_enumeration_master_done] (0x0400): Found flat name [(null)].
>>>
>>> We should print that we failed to get flat name but we will continue anyway.
>>
>> I don't think it will, we won't get to this line at all but rather jump
>
> Yes it will, I copied the debug message from the logs :-)
>
> This debug message is at three places:
> ad_domain_info.c:384 (area of your patch) this one is not printed
> ad_id.c:433
> ad_subdomains.c:374
>
> The last two are written into logs since you return ret = EOK.
Ah, you're right, thanks. But since the information is available in the
domain info request and the debug messages are already printed there, I
simply removed the debug statements from ad_id.c and ad_subdomains.c
Thanks for the review, new patches are attached.