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

Pavel Reichl preichl at redhat.com
Mon Jul 14 09:40:26 UTC 2014


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?

> 
>              }
>  
> -- 
> 1.7.11.7





More information about the sssd-devel mailing list