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.
to the done label using one of the previous if-s. But when reading
the
function again, I found a bad indentation, so attached is a revised
patch.