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

Pavel Reichl preichl at redhat.com
Mon Jul 14 10:08:29 UTC 2014


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.

>                  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





More information about the sssd-devel mailing list