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(a)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(a)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(a)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(a)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(a)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(a)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