[SSSD] [PATCH] resolv: fix server sort by weight

Pavel Březina pbrezina at redhat.com
Mon Jul 14 09:49:47 UTC 2014


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
>>> sssd-devel at 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 at lists.fedorahosted.org
>> 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-resolv-tests-remove-ununused-variable-from-for-cyclu.patch
Type: text/x-patch
Size: 812 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140714/0b6b3eaa/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-resolv-tests-add-test-for-multiple-servers-with-zero.patch
Type: text/x-patch
Size: 2784 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140714/0b6b3eaa/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-resolv-fix-server-sort-by-weight.patch
Type: text/x-patch
Size: 1154 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140714/0b6b3eaa/attachment-0002.bin>


More information about the sssd-devel mailing list