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?= <pbrez...@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@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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
