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(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
>>>
>>> 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;
>> }
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel