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

[Pavel Březina]

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.

[Lukas Slebodnik]

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?= 
> 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 [e-mail]
============================================================================
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

[Pavel Březina]

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?= 
> > 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 [e-mail]
> ============================================================================
> 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.

[Lukas Slebodnik]

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?= 
> > > 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 [e-mail]
> > ============================================================================
> > 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?= 
> 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?= 
> 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?
> 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?= 
> 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?= 
> 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.

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

LS

[Simo Sorce]

On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
[..]
nitpicking some more given Lucas nacked.
> > +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}};

I think this can be done by simply writing:

struct ares_srv_reply dummy[3] = { 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.
>
> 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

Why not simply make this a doubly linked list and use the functions in
src/util/dlinklist.h ?
Or if you really have to use single linked, maybe add a slinklist.h file
in util/ that does things as much as possible like dlinklist.h so that
we use the same style.

Simo.

[Pavel Březina]

On 08/08/2013 03:32 AM, Simo Sorce wrote:
> On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
> [..]
> nitpicking some more given Lucas nacked.
> > > +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}};
>
> I think this can be done by simply writing:
>
> struct ares_srv_reply dummy[3] = { 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.
> >
> > 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
>
> Why not simply make this a doubly linked list and use the functions in
> src/util/dlinklist.h ?

I'm working with struct ares_srv_reply that comes from ares api.
> Or if you really have to use single linked, maybe add a slinklist.h file
> in util/ that does things as much as possible like dlinklist.h so that
> we use the same style.

Done.

[Jakub Hrozek]

On Thu, Aug 08, 2013 at 01:24:17PM +0200, Pavel Březina wrote:
> On 08/08/2013 03:32 AM, Simo Sorce wrote:
> > On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
> > [..]
> > nitpicking some more given Lucas nacked.
> > > > +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}};
> >
> > I think this can be done by simply writing:
> >
> > struct ares_srv_reply dummy[3] = { 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.
> > >
> > > 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
> >
> > Why not simply make this a doubly linked list and use the functions in
> > src/util/dlinklist.h ?
>
> I'm working with struct ares_srv_reply that comes from ares api.
> > Or if you really have to use single linked, maybe add a slinklist.h file
> > in util/ that does things as much as possible like dlinklist.h so that
> > we use the same style.
>
> Done.

I was wondering if we make the ordering plugin a property of the SRV
lookup plugin and set it either using fo_set_srv_lookup_plugin() or with
a new function? In general, I think that the sorting would be only used
by the same provider as the lookup plugin, so it would be nice if we
could avoid new parameters for a generic function and keep the API
clean.

[Pavel Březina]

On 08/23/2013 05:44 PM, Jakub Hrozek wrote:
> On Thu, Aug 08, 2013 at 01:24:17PM +0200, Pavel Březina wrote:
> > On 08/08/2013 03:32 AM, Simo Sorce wrote:
> > > On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
> > > [..]
> > > nitpicking some more given Lucas nacked.
> > > > > +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}};
> > >
> > > I think this can be done by simply writing:
> > >
> > > struct ares_srv_reply dummy[3] = { 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.
> > > >
> > > > 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
> > >
> > > Why not simply make this a doubly linked list and use the functions in
> > > src/util/dlinklist.h ?
> >
> > I'm working with struct ares_srv_reply that comes from ares api.
> > > Or if you really have to use single linked, maybe add a slinklist.h file
> > > in util/ that does things as much as possible like dlinklist.h so that
> > > we use the same style.
> >
> > Done.
>
> I was wondering if we make the ordering plugin a property of the SRV
> lookup plugin and set it either using fo_set_srv_lookup_plugin() or with
> a new function? In general, I think that the sorting would be only used
> by the same provider as the lookup plugin, so it would be nice if we
> could avoid new parameters for a generic function and keep the API
> clean.

Hmm, then I guess I can move the whole functionality into the plugin 
itself. Instead of sorting ares result in fo_discover_srv*, we can sort 
fo_server_info in the AD plugin. It will require some changes I wanted 
to avoid, but its doable.

Does this sound right?

[Jakub Hrozek]

On Fri, Aug 23, 2013 at 07:36:22PM +0200, Pavel Březina wrote:
> On 08/23/2013 05:44 PM, Jakub Hrozek wrote:
> > On Thu, Aug 08, 2013 at 01:24:17PM +0200, Pavel Březina wrote:
> > > On 08/08/2013 03:32 AM, Simo Sorce wrote:
> > > > On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
> > > > [..]
> > > > nitpicking some more given Lucas nacked.
> > > > > > +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}};
> > > >
> > > > I think this can be done by simply writing:
> > > >
> > > > struct ares_srv_reply dummy[3] = { 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.
> > > > >
> > > > > 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
> > > >
> > > > Why not simply make this a doubly linked list and use the functions in
> > > > src/util/dlinklist.h ?
> > >
> > > I'm working with struct ares_srv_reply that comes from ares api.
> > > > Or if you really have to use single linked, maybe add a slinklist.h file
> > > > in util/ that does things as much as possible like dlinklist.h so that
> > > > we use the same style.
> > >
> > > Done.
> >
> > I was wondering if we make the ordering plugin a property of the SRV
> > lookup plugin and set it either using fo_set_srv_lookup_plugin() or with
> > a new function? In general, I think that the sorting would be only used
> > by the same provider as the lookup plugin, so it would be nice if we
> > could avoid new parameters for a generic function and keep the API
> > clean.
>
> Hmm, then I guess I can move the whole functionality into the plugin
> itself. Instead of sorting ares result in fo_discover_srv*, we can
> sort fo_server_info in the AD plugin. It will require some changes I
> wanted to avoid, but its doable.
>
> Does this sound right?

I would prefer this way but you know the failover code better now
probably. So what are the changes you wanted to avoid?

[Pavel Březina]

On 08/23/2013 07:39 PM, Jakub Hrozek wrote:
> On Fri, Aug 23, 2013 at 07:36:22PM +0200, Pavel Březina wrote:
> > On 08/23/2013 05:44 PM, Jakub Hrozek wrote:
> > > On Thu, Aug 08, 2013 at 01:24:17PM +0200, Pavel Březina wrote:
> > > > On 08/08/2013 03:32 AM, Simo Sorce wrote:
> > > > > On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
> > > > > [..]
> > > > > nitpicking some more given Lucas nacked.
> > > > > > > +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}};
> > > > >
> > > > > I think this can be done by simply writing:
> > > > >
> > > > > struct ares_srv_reply dummy[3] = { 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.
> > > > > >
> > > > > > 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
> > > > >
> > > > > Why not simply make this a doubly linked list and use the functions in
> > > > > src/util/dlinklist.h ?
> > > >
> > > > I'm working with struct ares_srv_reply that comes from ares api.
> > > > > Or if you really have to use single linked, maybe add a slinklist.h file
> > > > > in util/ that does things as much as possible like dlinklist.h so that
> > > > > we use the same style.
> > > >
> > > > Done.
> > >
> > > I was wondering if we make the ordering plugin a property of the SRV
> > > lookup plugin and set it either using fo_set_srv_lookup_plugin() or with
> > > a new function? In general, I think that the sorting would be only used
> > > by the same provider as the lookup plugin, so it would be nice if we
> > > could avoid new parameters for a generic function and keep the API
> > > clean.
> >
> > Hmm, then I guess I can move the whole functionality into the plugin
> > itself. Instead of sorting ares result in fo_discover_srv*, we can
> > sort fo_server_info in the AD plugin. It will require some changes I
> > wanted to avoid, but its doable.
> >
> > Does this sound right?
>
> I would prefer this way but you know the failover code better now
> probably. So what are the changes you wanted to avoid?

Well I either will have to change fo_server_info from array to dlist or 
rewrite the whole sorting function. At the moment, I prefer the latter 
one. :-)

But I have another idea: how about we make it part of default SRV 
sorting? Preferring those servers that are in the same DNS domain makes 
sense also for LDAP and IPA provider.

[Jakub Hrozek]

On Mon, Aug 26, 2013 at 12:08:56PM +0200, Pavel Březina wrote:
> On 08/23/2013 07:39 PM, Jakub Hrozek wrote:
> > On Fri, Aug 23, 2013 at 07:36:22PM +0200, Pavel Březina wrote:
> > > On 08/23/2013 05:44 PM, Jakub Hrozek wrote:
> > > > On Thu, Aug 08, 2013 at 01:24:17PM +0200, Pavel Březina wrote:
> > > > > On 08/08/2013 03:32 AM, Simo Sorce wrote:
> > > > > > On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
> > > > > > [..]
> > > > > > nitpicking some more given Lucas nacked.
> > > > > >
> > > > > > >>+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}};
> > > > > >
> > > > > > I think this can be done by simply writing:
> > > > > >
> > > > > > struct ares_srv_reply dummy[3] = { 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.
> > > > > > >
> > > > > > >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
> > > > > >
> > > > > > Why not simply make this a doubly linked list and use the functions in
> > > > > > src/util/dlinklist.h ?
> > > > >
> > > > > I'm working with struct ares_srv_reply that comes from ares api.
> > > > > > Or if you really have to use single linked, maybe add a slinklist.h file
> > > > > > in util/ that does things as much as possible like dlinklist.h so that
> > > > > > we use the same style.
> > > > >
> > > > > Done.
> > > >
> > > > I was wondering if we make the ordering plugin a property of the SRV
> > > > lookup plugin and set it either using fo_set_srv_lookup_plugin() or with
> > > > a new function? In general, I think that the sorting would be only used
> > > > by the same provider as the lookup plugin, so it would be nice if we
> > > > could avoid new parameters for a generic function and keep the API
> > > > clean.
> > >
> > > Hmm, then I guess I can move the whole functionality into the plugin
> > > itself. Instead of sorting ares result in fo_discover_srv*, we can
> > > sort fo_server_info in the AD plugin. It will require some changes I
> > > wanted to avoid, but its doable.
> > >
> > > Does this sound right?
> >
> > I would prefer this way but you know the failover code better now
> > probably. So what are the changes you wanted to avoid?
>
> Well I either will have to change fo_server_info from array to dlist
> or rewrite the whole sorting function. At the moment, I prefer the
> latter one. :-)
>
> But I have another idea: how about we make it part of default SRV
> sorting? Preferring those servers that are in the same DNS domain
> makes sense also for LDAP and IPA provider.

Yes, I don't have a problem with that. As long as priority and weight
are honored.

[Pavel Březina]

On 08/26/2013 12:15 PM, Jakub Hrozek wrote:
> On Mon, Aug 26, 2013 at 12:08:56PM +0200, Pavel Březina wrote:
> > On 08/23/2013 07:39 PM, Jakub Hrozek wrote:
> > > On Fri, Aug 23, 2013 at 07:36:22PM +0200, Pavel Březina wrote:
> > > > On 08/23/2013 05:44 PM, Jakub Hrozek wrote:
> > > > > On Thu, Aug 08, 2013 at 01:24:17PM +0200, Pavel Březina wrote:
> > > > > > On 08/08/2013 03:32 AM, Simo Sorce wrote:
> > > > > > > On Wed, 2013-08-07 at 13:56 +0200, Lukas Slebodnik wrote:
> > > > > > > [..]
> > > > > > > nitpicking some more given Lucas nacked.
> > > > > > >
> > > > > > >>> +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}};
> > > > > > >
> > > > > > > I think this can be done by simply writing:
> > > > > > >
> > > > > > > 	struct ares_srv_reply dummy[3] = { 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.
> > > > > > >>
> > > > > > >> 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
> > > > > > >
> > > > > > > Why not simply make this a doubly linked list and use the functions in
> > > > > > > src/util/dlinklist.h ?
> > > > > >
> > > > > > I'm working with struct ares_srv_reply that comes from ares api.
> > > > > >
> > > > > > > Or if you really have to use single linked, maybe add a slinklist.h file
> > > > > > > in util/ that does things as much as possible like dlinklist.h so that
> > > > > > > we use the same style.
> > > > > >
> > > > > > Done.
> > > > >
> > > > > I was wondering if we make the ordering plugin a property of the SRV
> > > > > lookup plugin and set it either using fo_set_srv_lookup_plugin() or with
> > > > > a new function? In general, I think that the sorting would be only used
> > > > > by the same provider as the lookup plugin, so it would be nice if we
> > > > > could avoid new parameters for a generic function and keep the API
> > > > > clean.
> > > >
> > > > Hmm, then I guess I can move the whole functionality into the plugin
> > > > itself. Instead of sorting ares result in fo_discover_srv*, we can
> > > > sort fo_server_info in the AD plugin. It will require some changes I
> > > > wanted to avoid, but its doable.
> > > >
> > > > Does this sound right?
> > >
> > > I would prefer this way but you know the failover code better now
> > > probably. So what are the changes you wanted to avoid?
> >
> > Well I either will have to change fo_server_info from array to dlist
> > or rewrite the whole sorting function. At the moment, I prefer the
> > latter one. :-)
> >
> > But I have another idea: how about we make it part of default SRV
> > sorting? Preferring those servers that are in the same DNS domain
> > makes sense also for LDAP and IPA provider.
>
> Yes, I don't have a problem with that. As long as priority and weight
> are honored.

I have done it AD only after all. Enabling it for all providers required 
more changes than expected. I don't think those changes should be done 
in this state of development...

[Jakub Hrozek]

On Mon, Aug 26, 2013 at 04:26:09PM +0200, Pavel Březina wrote:
> > Yes, I don't have a problem with that. As long as priority and weight
> > are honored.
>
> I have done it AD only after all. Enabling it for all providers
> required more changes than expected. I don't think those changes
> should be done in this state of development...

I like the new approach.
> From 9c03180cadcc8bde2da5037156034ba4591a6c15 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 2 Aug 2013 23:07:53 +0200
> Subject: [PATCH 1/4] resolv_sort_srv_reply: remove unnecessary mem_ctx

ACK. This function is also unit-tested and the tests include leak
checks.
> From 093a7292376593927b4bbaea29ccc208e83ff1a6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 26 Aug 2013 11:52:42 +0200
> Subject: [PATCH 2/4] fo srv: add priority and weight to fo_server_info
>
> This will give SRV plugins all information needed for additional
> sorting.

I don't see the weight used anywhere except the place where they are
assigned.
> From dd634784178fad7a2c98d06a5c2316f546a9727a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Sat, 3 Aug 2013 01:02:02 +0200
> Subject: [PATCH 3/4] utils: add is_host_in_domain()

ACK
> From 36610e77be90227e5e8c02fd1ea2192e3307059b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 26 Aug 2013 16:02:51 +0200
> Subject: [PATCH 4/4] ad srv: prefer servers that are in the same domain as
>  client
>
> https://fedorahosted.org/sssd/ticket/2001

ACK

[Pavel Březina]

On 09/04/2013 10:01 AM, Jakub Hrozek wrote:
> On Mon, Aug 26, 2013 at 04:26:09PM +0200, Pavel Březina wrote:
> > > Yes, I don't have a problem with that. As long as priority and weight
> > > are honored.
> >
> > I have done it AD only after all. Enabling it for all providers
> > required more changes than expected. I don't think those changes
> > should be done in this state of development...
>
> I like the new approach.
> > From 9c03180cadcc8bde2da5037156034ba4591a6c15 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Fri, 2 Aug 2013 23:07:53 +0200
> > Subject: [PATCH 1/4] resolv_sort_srv_reply: remove unnecessary mem_ctx
>
> ACK. This function is also unit-tested and the tests include leak
> checks.
> > From 093a7292376593927b4bbaea29ccc208e83ff1a6 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 26 Aug 2013 11:52:42 +0200
> > Subject: [PATCH 2/4] fo srv: add priority and weight to fo_server_info
> >
> > This will give SRV plugins all information needed for additional
> > sorting.
>
> I don't see the weight used anywhere except the place where they are
> assigned.
> > From dd634784178fad7a2c98d06a5c2316f546a9727a Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Sat, 3 Aug 2013 01:02:02 +0200
> > Subject: [PATCH 3/4] utils: add is_host_in_domain()
>
> ACK
> > From 36610e77be90227e5e8c02fd1ea2192e3307059b Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 26 Aug 2013 16:02:51 +0200
> > Subject: [PATCH 4/4] ad srv: prefer servers that are in the same domain as
> >   client
> >
> > https://fedorahosted.org/sssd/ticket/2001
>
> ACK

Thanks for the review. New weightless patches are attached.

[Pavel Březina]

On 09/04/2013 10:23 AM, Pavel Březina wrote:
> On 09/04/2013 10:01 AM, Jakub Hrozek wrote:
> > On Mon, Aug 26, 2013 at 04:26:09PM +0200, Pavel Březina wrote:
> > > > Yes, I don't have a problem with that. As long as priority and weight
> > > > are honored.
> > >
> > > I have done it AD only after all. Enabling it for all providers
> > > required more changes than expected. I don't think those changes
> > > should be done in this state of development...
> >
> > I like the new approach.
> > > From 9c03180cadcc8bde2da5037156034ba4591a6c15 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 2 Aug 2013 23:07:53 +0200
> > > Subject: [PATCH 1/4] resolv_sort_srv_reply: remove unnecessary mem_ctx
> >
> > ACK. This function is also unit-tested and the tests include leak
> > checks.
> > > From 093a7292376593927b4bbaea29ccc208e83ff1a6 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 26 Aug 2013 11:52:42 +0200
> > > Subject: [PATCH 2/4] fo srv: add priority and weight to fo_server_info
> > >
> > > This will give SRV plugins all information needed for additional
> > > sorting.
> >
> > I don't see the weight used anywhere except the place where they are
> > assigned.
> > > From dd634784178fad7a2c98d06a5c2316f546a9727a Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Sat, 3 Aug 2013 01:02:02 +0200
> > > Subject: [PATCH 3/4] utils: add is_host_in_domain()
> >
> > ACK
> > > From 36610e77be90227e5e8c02fd1ea2192e3307059b Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 26 Aug 2013 16:02:51 +0200
> > > Subject: [PATCH 4/4] ad srv: prefer servers that are in the same
> > > domain as
> > >   client
> > >
> > > https://fedorahosted.org/sssd/ticket/2001
> >
> > ACK
>
> Thanks for the review. New weightless patches are attached.

Oops, I didn't save the header file. Another respin is attached.

Also if anyone will ever need slinklist implementation, I have it backed 
up at: fedorapeople.org:public_git/sssd_unused.git in slinklist branch.

[Jakub Hrozek]

On Wed, Sep 04, 2013 at 10:42:43AM +0200, Pavel Březina wrote:
> > Thanks for the review. New weightless patches are attached.
>
> Oops, I didn't save the header file. Another respin is attached.
>
> Also if anyone will ever need slinklist implementation, I have it
> backed up at: fedorapeople.org:public_git/sssd_unused.git in
> slinklist branch.

I like how you put the patch on diet. ACK to all.

[Jakub Hrozek]

On Wed, Sep 04, 2013 at 10:54:55AM +0200, Jakub Hrozek wrote:
> On Wed, Sep 04, 2013 at 10:42:43AM +0200, Pavel Březina wrote:
> > > Thanks for the review. New weightless patches are attached.
> >
> > Oops, I didn't save the header file. Another respin is attached.
> >
> > Also if anyone will ever need slinklist implementation, I have it
> > backed up at: fedorapeople.org:public_git/sssd_unused.git in
> > slinklist branch.
>
> I like how you put the patch on diet. ACK to all.

Pushed to master.

[Pavel Březina]

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?= 
> > > > 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 [e-mail]
> > > ============================================================================
> > > 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?= 
> > 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?= 
> > 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?= 
> > 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?= 
> > 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