On 07/14/2014 12:08 PM, Pavel Reichl wrote:
On Mon, 2014-07-14 at 11:49 +0200, Pavel Březina wrote:
On 07/14/2014 11:40 AM, Pavel Reichl wrote:
On Mon, 2014-07-14 at 10:59 +0200, Pavel Březina wrote:
On 07/14/2014 10:42 AM, Pavel Reichl wrote:
On Thu, 2014-07-10 at 13:39 +0200, Pavel Březina wrote:
Hi,
thanks for the unit test.
Would you consider braking test into smaller part to make it possible to
run it over different sets of test data?
Different set of data is not necessary for this test. The problem only
occurs if you have two servers with the same priority and weight 0. And
that is exactly what the test tests.
Please see comments inline.
Thanks,
Pavel R
+START_TEST(test_resolv_sort_srv_reply_zero_weight)
+{
+ int ret;
+ struct ares_srv_reply *replies = NULL;
+ struct ares_srv_reply *r, *prev = NULL;
+ struct resolv_test_ctx *test_ctx;
+ int num_replies = 6;
+ int i;
+
+ ret = setup_resolv_test(RESOLV_DEFAULT_TIMEOUT, &test_ctx);
+ if (ret != EOK) {
+ fail("Could not set up test");
+ return;
+ }
+
+ ck_leaks_push(test_ctx);
+
+ /* prepare linked list with reversed values */
^^^^^^^^
How do you mean reverse? Do you mean descending order? I think priorities are
rather in ascending order something like this:
(replies,0) -> (_,0) -> (_,10) -> (_,10) -> (_,20) -> (_,20)
If I am right could you amend the comment?
Copy and paste error, the comment is invalid :-)
+ for (i = 0; i < num_replies; i++) {
+ r = talloc_zero(test_ctx, struct ares_srv_reply);
+ fail_if(r == NULL);
+
+ r->priority = 20;
+ r->priority = i <= 3 ? 10 : r->priority;
+ r->priority = i <= 1 ? 0 : r->priority;
+ r->weight = 0;
+
+ if (replies == NULL) {
+ replies = r;
+ prev = r;
+ } else {
+ prev->next = r;
+ prev = prev->next;
Would it make sense to use here macros from DLIST?
Ah...I see it's not used anywhere in this module anyway, so I guess it would be
inconsistent to start using it here.
Nope, the servers are stored in forward connected list, so DLIST
wouldn't work. I prepared similar set of macros for this kind of list
some time ago, but it wasn't accepted that time.
+ }
+ }
+
+ /* do the sort */
+ ret = resolv_sort_srv_reply(&replies);
Are you sure about the input data? I think it's already sorted, is it on
purpose?
I think it's perfectly OK to test it on sorted data, but I would also like to
see it run on mixed data, would it make sense to you?
This test is not about sorting (another test for this exists already).
The problem is that if you have two or more servers with the same
priority and weight 0, only one of the servers is returned and the
others are missing.
+ fail_if(ret != EOK);
+
+ /* check if the list contains all values and is sorted */
+ for (i = 0, r = replies; r != NULL; r = r->next, i++) {
+ if (r->next != NULL) {
+ fail_unless(r->priority <= r->next->priority);
+ }
+ }
+ fail_unless(i == num_replies);
+
+ /* clean up */
+ prev = NULL;
+ for (i = 1, r = replies; r != NULL; r=r->next, i++) {
Do I miss something or is 'i' used in for-loop by accident?
Copy and paste error. I also fixed this in the other test.
+ talloc_zfree(prev);
+ prev = r;
+ }
+ talloc_zfree(prev);
+
+
+ /* check for leaks */
+ ck_leaks_pop(test_ctx);
+ talloc_zfree(test_ctx);
+}
+END_TEST
+
START_TEST(test_resolv_free_req)
{
int ret = EOK;
@@ -904,6 +967,7 @@ Suite *create_resolv_suite(void)
tcase_add_test(tc_resolv, test_address_to_string);
tcase_add_test(tc_resolv, test_resolv_ip_addr);
tcase_add_test(tc_resolv, test_resolv_sort_srv_reply);
+ tcase_add_test(tc_resolv, test_resolv_sort_srv_reply_zero_weight);
if (use_net_test) {
tcase_add_test(tc_resolv, test_resolv_internet);
tcase_add_test(tc_resolv, test_resolv_negative);
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for the review, new patches are attached.
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thank you for clarification, I have one more question:
diff --git a/src/resolv/async_resolv.c b/src/resolv/async_resolv.c
index
38fa84d9f14a3d2f850feefa2c3076ae2618e659..d9538ce04452734ca2e2784d6a3073d85411b15b
100644
--- a/src/resolv/async_resolv.c
+++ b/src/resolv/async_resolv.c
@@ -2093,11 +2093,11 @@ static int reply_weight_rearrange(int len,
r = *(start);
prev = NULL;
while (r != NULL) {
- if (r->weight == 0) {
+ if (r->weight == 0 && r != *start) {
/* remove from the old list */
if (prev) {
prev->next = r->next;
- } else {
+ } else if (r->next != NULL) {
*start = r->next;
Do we ever enter to this branch? I mean prev is always != NULL, right? If it's
so could you simplify this code?
Nice catch. It really became a dead code with the first change.
Thanks, just the last nitpick:
while (r != NULL) {
- if (r->weight == 0) {
+ if (r->weight == 0 && r != *start) {
/* remove from the old list */
if (prev) {
Do you really need the condition here? I think that prev is *always* not null,
but if this condition makes you feel more comfortable I'll ack the patches
anyway.
Ok, I removed it.
prev->next = r->next;
- } else {
- *start = r->next;
}
From b042508aafa548a7116e7f144b940701d35d1eae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]>
Date: Mon, 14 Jul 2014 10:58:15 +0200
Subject: [PATCH 1/3] resolv tests: remove ununused variable from for cyclus
---
src/tests/resolv-tests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tests/resolv-tests.c b/src/tests/resolv-tests.c
index 5b85bf6ef45485444442f029d62afc9795037137..fa6e6367760c6877e7fe672c2c25b8a3cb998d3d 100644
--- a/src/tests/resolv-tests.c
+++ b/src/tests/resolv-tests.c
@@ -767,7 +767,7 @@ START_TEST(test_resolv_sort_srv_reply)
/* clean up */
prev = NULL;
- for (i = 1, r = replies; r; r=r->next, i++) {
+ for (r = replies; r; r=r->next) {
talloc_zfree(prev);
prev = r;
}
--
1.7.11.7
From e1c55d875e93bdb10b341037ec2a491d6693090a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]>
Date: Thu, 10 Jul 2014 13:36:42 +0200
Subject: [PATCH 2/3] resolv tests: add test for multiple servers with zero
weights
Resolves:
https://fedorahosted.org/sssd/ticket/2357
---
src/tests/resolv-tests.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/src/tests/resolv-tests.c b/src/tests/resolv-tests.c
index fa6e6367760c6877e7fe672c2c25b8a3cb998d3d..30d551f636b73605ab3daabf5bdddf2c5884b5a2 100644
--- a/src/tests/resolv-tests.c
+++ b/src/tests/resolv-tests.c
@@ -780,6 +780,69 @@ START_TEST(test_resolv_sort_srv_reply)
}
END_TEST
+START_TEST(test_resolv_sort_srv_reply_zero_weight)
+{
+ int ret;
+ struct ares_srv_reply *replies = NULL;
+ struct ares_srv_reply *r, *prev = NULL;
+ struct resolv_test_ctx *test_ctx;
+ int num_replies = 6;
+ int i;
+
+ ret = setup_resolv_test(RESOLV_DEFAULT_TIMEOUT, &test_ctx);
+ if (ret != EOK) {
+ fail("Could not set up test");
+ return;
+ }
+
+ ck_leaks_push(test_ctx);
+
+ /* prepare linked list */
+ for (i = 0; i < num_replies; i++) {
+ r = talloc_zero(test_ctx, struct ares_srv_reply);
+ fail_if(r == NULL);
+
+ r->priority = 20;
+ r->priority = i <= 3 ? 10 : r->priority;
+ r->priority = i <= 1 ? 0 : r->priority;
+ r->weight = 0;
+
+ if (replies == NULL) {
+ replies = r;
+ prev = r;
+ } else {
+ prev->next = r;
+ prev = prev->next;
+ }
+ }
+
+ /* do the sort */
+ ret = resolv_sort_srv_reply(&replies);
+ fail_if(ret != EOK);
+
+ /* check if the list contains all values and is sorted */
+ for (i = 0, r = replies; r != NULL; r = r->next, i++) {
+ if (r->next != NULL) {
+ fail_unless(r->priority <= r->next->priority);
+ }
+ }
+ fail_unless(i == num_replies);
+
+ /* clean up */
+ prev = NULL;
+ for (r = replies; r != NULL; r=r->next) {
+ talloc_zfree(prev);
+ prev = r;
+ }
+ talloc_zfree(prev);
+
+
+ /* check for leaks */
+ ck_leaks_pop(test_ctx);
+ talloc_zfree(test_ctx);
+}
+END_TEST
+
START_TEST(test_resolv_free_req)
{
int ret = EOK;
@@ -904,6 +967,7 @@ Suite *create_resolv_suite(void)
tcase_add_test(tc_resolv, test_address_to_string);
tcase_add_test(tc_resolv, test_resolv_ip_addr);
tcase_add_test(tc_resolv, test_resolv_sort_srv_reply);
+ tcase_add_test(tc_resolv, test_resolv_sort_srv_reply_zero_weight);
if (use_net_test) {
tcase_add_test(tc_resolv, test_resolv_internet);
tcase_add_test(tc_resolv, test_resolv_negative);
--
1.7.11.7
From 213b9513ee8959d6a4a34235bf4c1ddce924ffa2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <[email protected]>
Date: Thu, 10 Jul 2014 13:37:37 +0200
Subject: [PATCH 3/3] resolv: fix server sort by weight
When the server list consist only from servers with zero weight the
output list contained only one server.
Resolves:
https://fedorahosted.org/sssd/ticket/2357
---
src/resolv/async_resolv.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/resolv/async_resolv.c b/src/resolv/async_resolv.c
index 38fa84d9f14a3d2f850feefa2c3076ae2618e659..f93612440ec8f9ce393bede8316b512e46d577bc 100644
--- a/src/resolv/async_resolv.c
+++ b/src/resolv/async_resolv.c
@@ -2093,13 +2093,9 @@ static int reply_weight_rearrange(int len,
r = *(start);
prev = NULL;
while (r != NULL) {
- if (r->weight == 0) {
+ if (r->weight == 0 && r != *start) {
/* remove from the old list */
- if (prev) {
- prev->next = r->next;
- } else {
- *start = r->next;
- }
+ prev->next = r->next;
/* add to the head of the new list */
tmp = r;
--
1.7.11.7
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
