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

Pavel Březina pbrezina at redhat.com
Mon Jul 14 10:15:48 UTC 2014


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
>>>>> 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.
>
> 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;
>>               }

-------------- 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/45d68705/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/45d68705/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: 1215 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140714/45d68705/attachment-0002.bin>


More information about the sssd-devel mailing list