[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