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

Pavel Reichl preichl at redhat.com
Mon Jul 14 12:16:13 UTC 2014


On Mon, 2014-07-14 at 12:15 +0200, Pavel Březina wrote:
> 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;
> >>               }
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thanks, ACK to all three patches.






More information about the sssd-devel mailing list