[PATCH] AD: Failure to get flat name is not fatal

[Jakub Hrozek]

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.

[Pavel Březina]

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)
> 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?
> 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.
> 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;
> }

[Jakub Hrozek]

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.

[Pavel Březina]

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.

[Jakub Hrozek]

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
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.

[Pavel Březina]

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.
> 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.

[Jakub Hrozek]

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.

[Pavel Březina]

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.

[Jakub Hrozek]

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