Hi Jakub, thanks for review. I have just a few little remarks, please see inline.
On Tue, 2013-11-26 at 15:23 +0100, Jakub Hrozek wrote: > On Tue, Nov 19, 2013 at 10:14:49PM +0100, Pavel Reichl wrote: > > On Fri, 2013-11-15 at 21:01 +0100, Jakub Hrozek wrote: > > > On Fri, Nov 15, 2013 at 11:15:53AM +0100, Pavel Reichl wrote: > > > > Hello, > > > > > > > > First patch improves domain detection taking matched length into account > > > > as proposed and elaborated by Jakub. > > > > > > > > Second patch is a simple unit test testing sss_ldap_dn_in_search_bases > > > > and sdap_domain_get_by_dn. > > > > > > > > Pavel Reichl > > > > > > Hi Pavel, > > > > > > I haven't really read the full patches yet, just scrolled through them, > > > so I have only one comment so far: > > > > > > > +++ b/src/tests/cmocka/test_search_bases.c > > > > @@ -0,0 +1,214 @@ > > > > +/* > > > > + SSSD > > > > + > > > > + find_uid - Utilities tests > > > > + > > > > + Authors: > > > > + Abhishek Singh <abhishekkumarsingh....@gmail.com> > > > > + > > > > + Copyright (C) 2013 Red Hat > > > > > > You should credit yourself :-) > > > > Hi Jakub, > > > > you are right, thanks for noticing. > > Hi, in general the patches are working well and looking good. I only have > a couple of comments, see inline. > > > From c7632a2310442cfc10708c5728bd63e6a8c916d2 Mon Sep 17 00:00:00 2001 > > From: Pavel Reichl <pavel.rei...@redhat.com> > > Date: Thu, 14 Nov 2013 21:34:51 +0000 > > Subject: [PATCH 1/2] SSSD: Improved domain detection > > > > A bit more elegant way of detection of what domain the group member belongs > > to > > > > Resolves: > > https://fedorahosted.org/sssd/ticket/2132 > > I only have code style comments about this patch. > > > + tmp_ctx = talloc_new(NULL); > > + if (tmp_ctx == NULL) > > + return NULL; > > We use brackets around single-line conditionals as well. OK, I have no trouble fixing this, but accordingly to http://www.freeipa.org/page/Coding_Style#IF_Statements I got an idea that statements like: if (foo) bar(); else baz(); are legal so I assumed that: if (foo) bar(); would be legal too. So it may be a good idea to clarify this in above mentioned document or maybe It's just my bad reading. :-) > > > > > DLIST_FOR_EACH(sditer, opts->sdom) { > > - if (strcasecmp(sditer->basedn, dc) == 0) { > > - return sditer; > > + if (sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, > > sditer->search_bases, NULL, > > + &match_len) > > Can you reformat the first call so it's within 80 characters? > > > [snip] > > > @@ -558,6 +563,8 @@ bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, > > * Append filter otherwise. > > */ > > ret = true; > > + if (_match_len) > > + *_match_len = match_len; > > Same comment about brackets here. > > > From cfcd83aca098784d9ab1ca59f0a1ad8ef3145b62 Mon Sep 17 00:00:00 2001 > > From: Pavel Reichl <pavel.rei...@redhat.com> > > Date: Thu, 14 Nov 2013 21:52:26 +0000 > > Subject: [PATCH] SSSD: Unit test - sss_ldap_dn_in_search_bases > > > +struct sdap_search_base** generate_bases(TALLOC_CTX *mem_ctx, const char** > > dns, size_t n) > > Can you make the helper functions static and wrap for 80 characters per > line? > > > +{ > > + struct sdap_search_base **search_bases; > > + errno_t err; > > + int i; > > + > > + search_bases = talloc_array(mem_ctx, struct sdap_search_base *, n + 1); > > + assert_non_null(search_bases); > > + > > + for(i=0; i < n; ++i) { > > + err = sdap_create_search_base(mem_ctx, dns[i], LDAP_SCOPE_SUBTREE, > > + NULL, &search_bases[i]); > > + if (err != EOK) { > > + fprintf(stderr, "Failed to create search base\n"); > > + } > > + assert_true(err == EOK); > > cmocka has helper macros for comparing two integers: > assert_int_equal(err, EOK); > > > + } > > + search_bases[n] = NULL; > > + return search_bases; > > +} > > + > > +bool do_test_search_bases(const char* dn, const char** dns, size_t n) > > +{ > > + TALLOC_CTX *tmp_ctx; > > + struct sdap_search_base **search_bases; > > + bool ret; > > + > > + tmp_ctx = talloc_new(NULL); > > + assert_non_null(tmp_ctx); > > + > > + search_bases = generate_bases(tmp_ctx, dns, n); > > Can you check how we use check_leaks_push() and check_leaks_pop() > elsewhere? I think it would be nice to assert that > sss_ldap_dn_in_search_bases() does not allocate more memory that it > should. > > > + ret = sss_ldap_dn_in_search_bases(tmp_ctx, dn, search_bases, NULL); > > + > > + talloc_free(tmp_ctx); > > + return ret; > > +} > > + > > +void test_search_bases_fail(void **state) > > +{ > > + const char *dn = "cn=user, dc=sub, dc=ad, dc=pb"; > > + const char *dns[] = {"dc=example, dc=com", "dc=subdom, dc=ad, dc=pb"}; > > + bool ret; > > + > > + ret = do_test_search_bases(dn, dns, 2); > > + assert_false(ret); > > +} > > + > > +void test_search_bases_success(void **state) > > +{ > > + const char *dn = "cn=user, dc=sub, dc=ad, dc=pb"; > > + const char *dns[] = {"", "dc=ad, dc=pb", "dc=sub, dc=ad, dc=pb"}; > > + bool ret; > > + > > + ret = do_test_search_bases(dn, dns, 3); > > + assert_true(ret); > > +} > > + > > +void test_search_empty_bases_fail(void **sstate) > > +{ > > + TALLOC_CTX *tmp_ctx; > > + struct sdap_search_base **search_bases; > > + const char *dn = "cn=user, dc=sub, dc=ad, dc=pb"; > > + errno_t err; > > + bool ret; > > + > > + tmp_ctx = talloc_new(NULL); > > + assert_non_null(tmp_ctx); > > + > > + search_bases = talloc_array(tmp_ctx, struct sdap_search_base*, 2); > > + assert_non_null(search_bases); > > + > > + err = sdap_create_search_base(tmp_ctx, "", LDAP_SCOPE_SUBTREE, NULL, > > Search base "" is kind of a special case in LDAP, I think it can only be > used to describe the special rootDSE entry and can't be used otherwise. > Maybe we should patch sdap_create_search_base to only allow a base with > strlen > 1. > > Was the test purpose to check a wrong search base or one that doesn't > match at all? Well I just thought it would be a good idea to test an edge case. I wasn't really sure about correct behavior for this case. I'll remove this test completely, OK? > > > + &search_bases[0]); > > + assert_true(err == EOK); > > + search_bases[1] = NULL; > > + > > + ret = sss_ldap_dn_in_search_bases(tmp_ctx, dn, search_bases, NULL); > > + assert_false(ret); > > + > > + talloc_free(tmp_ctx); > > +} > > + > > + > > +/* > > + * expected_result equal to 0 means that dn is not in any domain > > + * expected_result equal to 1 means that dn is in the domain based on dns > > + * expected_result equal to 2 means that dn is in the domain based on dns2 > > + */ > > I was thinking about converting the above into an enum, but it's > probably not worth it. > > > + > > +void do_test_get_by_dn(const char *dn, const char **dns, size_t n, > > + const char **dns2, size_t n2, int expected_result) > > +{ > > + TALLOC_CTX *tmp_ctx; > > + struct sdap_options *opts; > > + struct sdap_domain *sdom; > > + struct sdap_domain *sdom2; > > + struct sdap_domain *res_sdom; > > + struct sdap_search_base **search_bases; > > + struct sdap_search_base **search_bases2; > > + tmp_ctx = talloc_new(NULL); > > Parts of the unit test checks for allocation failures and part does not. > The code should choose one and stick with it (and checking allocation > failures is safer, even though in non-deamons not completely necessary). > > > + > > + search_bases = generate_bases(tmp_ctx, dns, n); > > + search_bases2 = generate_bases(tmp_ctx, dns2, n2); > > + sdom = talloc_zero(tmp_ctx, struct sdap_domain); > > + sdom2 = talloc_zero(tmp_ctx, struct sdap_domain); > > + > > + sdom->search_bases = search_bases; > > + sdom->next = sdom2; > > + sdom->prev = NULL; > > + sdom2->search_bases = search_bases2; > > + sdom2->next = NULL; > > + sdom2->prev = sdom; > > + > > + opts = talloc(tmp_ctx, struct sdap_options); > > + opts->sdom = sdom; > > + res_sdom = sdap_domain_get_by_dn(opts, dn); > > + > > + switch(expected_result) { > > + case 0: > > + assert_null(res_sdom); > > + break; > > + case 1: > > + assert_true(res_sdom == sdom); > > + break; > > + case 2: > > + assert_true(res_sdom == sdom2); > > + break; > > + } > > + talloc_free(tmp_ctx); > > +} > > + > > [snip] > > > + > > +int main(void) > > +{ > > + const UnitTest tests[] = { > > + unit_test(test_search_bases_fail), > > + unit_test(test_search_bases_success), > > +#if 0 > > + unit_test(test_search_empty_bases_fail), > > +#endif > > I don't like a test case that is commented out, we should either fix it > or remove it. I tried to address this issue above I suggest removing it. > > > + unit_test(test_get_by_dn_fail), > > + unit_test(test_get_by_dn), > > + unit_test(test_get_by_dn2) > > + }; > > + > > + return run_tests(tests); > > +} > > -- > > 1.8.3.1 > > > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
