On Fri, Sep 20, 2013 at 10:52:24AM +0200, Pavel Březina wrote: > On 09/19/2013 07:03 PM, Jakub Hrozek wrote: > >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. > > Ack.
Pushed to master and sssd-1-11 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
