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
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
Thanks for the review, new patches are attached.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel 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?