[PATCH] resolv: fix server sort by weight

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/2357

[Pavel Reichl]

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?

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?
> 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.
>    }
> }
> /* 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?
> 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?
>    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);

[Pavel Březina]

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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thanks for the review, new patches are attached.

[Pavel Reichl]

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
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Thanks for the review, new patches are attached.
>
> sssd-devel mailing list
> [e-mail]
> 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

[Pavel Březina]

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
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Thanks for the review, new patches are attached.
> >
> > sssd-devel mailing list
> > [e-mail]
> > 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.

[Pavel Reichl]

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
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > Thanks for the review, new patches are attached.
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > 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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

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
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > Thanks for the review, new patches are attached.
> > > >
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > 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;
> >         }

[Pavel Reichl]

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
> > > > > > [e-mail]
> > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > > > > Thanks for the review, new patches are attached.
> > > > >
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > 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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thanks, ACK to all three patches.

[Jakub Hrozek]

On Mon, Jul 14, 2014 at 02:16:13PM +0200, Pavel Reichl wrote:
> Thanks, ACK to all three patches.

Pushed to master:
 * 2be2220f15b2a819a80b07c8881822ef87053c69
 * da6ee4b16f57d24f63c09efa74ead7f7cf315a55
 * cb8c79278d23a5b1deb66ea4af37feebc7137fd0