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.
> prev->next = r->next;
> - } else {
> - *start = r->next;
> }