[SSSD] [PATCHES] ad srv: prefer servers that are in the same domain as client

Pavel Březina pbrezina at redhat.com
Thu Aug 8 11:00:52 UTC 2013


On 08/07/2013 01:56 PM, Lukas Slebodnik wrote:
> On (06/08/13 14:43), Pavel Březina wrote:
>> On 08/06/2013 10:20 AM, Lukas Slebodnik wrote:
>>> On (05/08/13 15:24), Pavel Březina wrote:
>>>> https://fedorahosted.org/sssd/ticket/2001
>>>>
>>>> There is a bug in underlying resolv_sort_srv_reply(). It sometimes
>>>> returns less servers than there is on the input. This seems to be
>>>> related to configuration when two servers has the same priority and
>>>> weight (0). I haven't found the root cause yet, I'll investigate
>>>> more.
>>>
>>> NACK
>>>
>>> >From ab328b91f0bdd5168a5d09f3748e5b2091c085b0 Mon Sep 17 00:00:00 2001
>>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>>> Date: Sat, 3 Aug 2013 01:02:02 +0200
>>>> Subject: [PATCH 3/7] Add is_host_in_domain() util function
>>>
>>> Tests failed with 3rd patch and last patch did not fix this.
>>>
>>> FAIL: util-tests
>>> ============================================================================
>>> Testsuite summary for sssd 1.10.93
>>> ============================================================================
>>> # TOTAL: 30
>>> # PASS:  29
>>> # SKIP:  0
>>> # XFAIL: 0
>>> # FAIL:  1
>>> # XPASS: 0
>>> # ERROR: 0
>>> ============================================================================
>>> See ./test-suite.log
>>> Please report to sssd-devel at lists.fedorahosted.org
>>> ============================================================================
>>> make[3]: *** [test-suite.log] Error 1
>>> make[3]: Leaving directory `/dev/shm/sssd_build'
>>> make[2]: *** [check-TESTS] Error 2
>>> make[2]: Leaving directory `/dev/shm/sssd_build'
>>> make[1]: *** [check-am] Error 2
>>> make[1]: Leaving directory `/dev/shm/sssd_build'
>>> make: *** [check-recursive] Error 1
>>>
>>> bash$ cat util-tests.log
>>> Running suite(s): util
>>> test_murmurhash3_random seed: 1375776937
>>> 95%: Checks: 20, Failures: 1, Errors: 0
>>> ../sssd/src/tests/util-tests.c:833:F:util:test_is_host_in_domain:0: Host: example.com, Domain: child.example.com, Expected: 0, Got: 1
>>>
>>> LS
>>
>> Thanks. I switch from int to size_t after running the tests and it
>> caused troubles, because size_t is obviously unsigned.
>>
>> New patches are attached.
>>
>
>>From 592f208dd35703fed7da813dcef7f61724d94edb Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Fri, 2 Aug 2013 23:07:53 +0200
>> Subject: [PATCH 1/5] resolv_sort_srv_reply: remove unnecessary mem_ctx
>
> ACK
>
>>From 978227dcd225948d58fdb00b501da7e8c230f9fd Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Thu, 1 Aug 2013 16:15:58 +0200
>> Subject: [PATCH 2/5] fo_discover_srv_send: allow custom ordering function
>>
>> +    state->sort_srv_fn = sort_srv_fn;
>> +    state->sort_srv_pvt = sort_srv_pvt;
>> +
>>      subreq = resolv_discover_srv_send(state, ev, resolv_ctx, service,
>>                                        protocol, discovery_domains);
>>      if (subreq == NULL) {
>> @@ -98,7 +106,11 @@ static void fo_discover_srv_done(struct tevent_req *subreq)
>>      DEBUG(SSSDBG_TRACE_FUNC, ("Got answer. Processing...\n"));
>>
>>      /* sort and store the answer */
>> -    ret = resolv_sort_srv_reply(&reply_list);
>> +    if (state->sort_srv_fn == NULL) {
>> +        ret = resolv_sort_srv_reply(&reply_list);
>> +    } else {
>> +        ret = state->sort_srv_fn(&reply_list, state->sort_srv_pvt);
>> +    }
> resolv_sort_srv_reply is called inside of ad_sort_servers
> (only one implenentation of sorting function state->sort_srv_fn)
> Would it be better to call resolv_sort_srv_reply here every time?

I had it originally like that, but I think it is better this way, In my 
opinion, if a sorting function is provided, it should not expect that 
the list is already presorted. Otherwise we may get into trouble with 
future changes.

>>      if (ret != EOK) {
>>          DEBUG(SSSDBG_CRIT_FAILURE, ("Could not sort the answers from DNS "
>>                                      "[%d]: %s\n", ret, strerror(ret)));
>> @@ -172,6 +184,8 @@ struct fo_discover_servers_state {
>>      const char *protocol;
>>      const char *primary_domain;
>>      const char *backup_domain;
>> +    fo_sort_srv_list_t sort_srv_fn;
>> +    void *sort_srv_pvt;
>>
>
>>From ff7cd0f69449d8b2382a041d4424ca0b2ef5410e Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Sat, 3 Aug 2013 01:02:02 +0200
>> Subject: [PATCH 3/5] Add is_host_in_domain() util function
>>
>> ---
>> src/tests/util-tests.c | 28 ++++++++++++++++++++++++++++
>> src/util/util.c        | 15 +++++++++++++++
>> src/util/util.h        |  2 ++
>> 3 files changed, 45 insertions(+)
> test++ :-). It is really good.
> ACK
>
>>From 7cd5f605121ab8af457400c13f7e6374a90535df Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Fri, 2 Aug 2013 23:01:57 +0200
>> Subject: [PATCH 4/5] ad srv: prefer servers that are in the same domain as
>> client
>>
>> https://fedorahosted.org/sssd/ticket/2001
>> ---
>> src/providers/ad/ad_srv.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 81 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/providers/ad/ad_srv.c b/src/providers/ad/ad_srv.c
>> index dfb15b30c634f2fc672d1072425dc48bdf6b7740..cd60cf6ee8669b32840a6156a04bbaf300ac222b 100644
>> --- a/src/providers/ad/ad_srv.c
>> +++ b/src/providers/ad/ad_srv.c
>> @@ -38,6 +38,15 @@
>> #define AD_AT_NT_VERSION "NtVer"
>> #define AD_AT_NETLOGON "netlogon"
>>
>> +#define APPEND_ARES_LIST(dst_tail, src_head, src_tail) do { \
>> +    if (src_head->next != NULL) { \
>> +        dst_tail->next = src_head->next; \
>> +        dst_tail = src_tail; \
>> +        src_head->next = NULL; \
>> +        src_tail = src_head; \
>> +    } \
>> +} while (0);
>> +
>> struct ad_get_dc_servers_state {
>>      struct fo_server_info *servers;
>>      size_t num_servers;
>> @@ -548,6 +557,77 @@ static void ad_srv_plugin_dcs_done(struct tevent_req *subreq);
>> static void ad_srv_plugin_site_done(struct tevent_req *subreq);
>> static void ad_srv_plugin_servers_done(struct tevent_req *subreq);
>>
>> +static errno_t ad_sort_servers(struct ares_srv_reply **list, void *pvt)
>> +{
>> +    struct ad_srv_plugin_state *state = NULL;
>> +    struct ares_srv_reply dummy[3] = {{NULL, NULL, 0, 0, 0}};
>> +    struct ares_srv_reply *head = &dummy[0];
>> +    struct ares_srv_reply *tail = &dummy[0];
>> +    struct ares_srv_reply *in_head = &dummy[1];
>> +    struct ares_srv_reply *in_tail = &dummy[1];
>> +    struct ares_srv_reply *out_head = &dummy[2];
>> +    struct ares_srv_reply *out_tail = &dummy[2];
>> +    struct ares_srv_reply *cur = NULL;
>> +    struct ares_srv_reply *next = NULL;
>> +    const char *domain = NULL;
>> +    errno_t ret;
>> +
>> +    if (list == NULL) {
>> +        ret = EINVAL;
>> +        goto done;
>> +    }
>> +
>> +    if (*list == NULL || (*list)->next == NULL) {
>> +        ret = EOK;
>> +        goto done;
>> +    }
>> +
>> +    state = talloc_get_type(pvt, struct ad_srv_plugin_state);
>> +    domain = state->discovery_domain;
>> +
>> +    /* first sort by rfc2782 */
>> +    ret = resolv_sort_srv_reply(list);
>> +    if (ret != EOK) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("resolv_sort_srv_reply() failed "
>> +                                    "[%d]: %s\n", ret, strerror(ret)));
>> +        goto done;
>> +    }
>> +
>> +    /* when several servers share priority, prefer the one that is located
>> +     * in the same domain as client (e.g. child domain instead of forest
>> +     * root) */
>> +
>> +    next = *list;
>> +    do {
>> +        cur = next;
>> +        next = cur->next;
>> +
>> +        if (is_host_in_domain(cur->host, domain)) {
>> +            in_tail->next = cur;
>> +            in_tail = in_tail->next;
>> +            in_tail->next = NULL;
>> +        } else {
>> +            out_tail->next = cur;
>> +            out_tail = out_tail->next;
>> +            out_tail->next = NULL;
>> +        }
>> +
>> +        if (next == NULL || cur->priority != next->priority) {
>> +            /* priority has changed or we have reached the end of the srv list,
>> +             * we will merge the lists */
>> +            APPEND_ARES_LIST(tail, in_head, in_tail);
>> +            APPEND_ARES_LIST(tail, out_head, out_tail);
>> +        }
>> +    } while (next != NULL);
>> +
>> +    *list = head->next;
>> +
>> +    ret = EOK;
>> +
>> +done:
>> +    return ret;
>> +}
>
> I am sorry, but this linked list sorting is not very readable.
> There is a lot of "struct ares_srv_reply" pointers:
>      head, tail, in_head, in_tail, out_head, out_tail, cur, next
> I tried to visualize this sorting with pencil and paper, but
> it was a real mess.

Hehe, it's actually quite simple - it is first level of quicksort, using 
domain as pivot. It is enough since the list is already presorted.

I will add some more comments and convert to similar api as dlinklist as 
Simo suggested.

>
> I would prefer to:
>    either: use similar approach like in reply_priority_sort
>            from file src/resolv/async_resolv.c (merger-sort)
>    or: don't change this function, but unit test should be written for this
>        function




More information about the sssd-devel mailing list