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.