On (27/11/13 15:24), Pavel Reichl wrote: >On Tue, 2013-11-26 at 18:25 +0100, Jakub Hrozek wrote: >> On Tue, Nov 26, 2013 at 05:28:18PM +0100, Pavel Reichl wrote: >> > 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. :-) >> >> Per IPA developers not using braces is discouraged. I agree the wiki >> page should be clarified. >> _______________________________________________ >> sssd-devel mailing list >> sssd-devel@lists.fedorahosted.org >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > >Hi, > >thanks for information. Improved patches are attached. > >PR >
>From 669ba173b0daaef99fab6ea1a8d5e5eb71c2cea1 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 > >From cdd879924d048de33b604a0730369e05cae8a813 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 2/2] SSSD: Unit test - sss_ldap_dn_in_search_bases > >Unit test testing detection of the right domain when processing group with >members from several domains > >Resolves: >https://fedorahosted.org/sssd/ticket/2132 >--- > Makefile.am | 27 ++++- > src/tests/cmocka/test_search_bases.c | 191 +++++++++++++++++++++++++++++++++++ > 2 files changed, 217 insertions(+), 1 deletion(-) > create mode 100644 src/tests/cmocka/test_search_bases.c > >diff --git a/Makefile.am b/Makefile.am >index >992d5796ea3e729238a8d7664c128112291c40ab..ed155bf25c1d1adfe91a7c3c76cdbce62bf6ae59 > 100644 >--- a/Makefile.am >+++ b/Makefile.am >@@ -154,7 +154,8 @@ if HAVE_CMOCKA > fqnames-tests \ > test_sss_idmap \ > test_utils \ >- ad_access_filter_tests >+ ad_access_filter_tests \ >+ test_search_bases > endif > > check_PROGRAMS = \ >@@ -1388,6 +1389,30 @@ test_utils_LDADD = \ > $(SSSD_INTERNAL_LTLIBS) \ > libsss_test_common.la > >+test_search_bases_SOURCES = \ >+ $(sssd_be_SOURCES) \ >+ src/util/sss_ldap.c \ >+ src/util/sss_krb5.c \ >+ src/util/find_uid.c \ >+ src/util/user_info_msg.c \ >+ src/tests/cmocka/test_search_bases.c >+test_search_bases_CFLAGS = \ >+ $(AM_CFLAGS) \ >+ -DUNIT_TESTING >+test_search_bases_LDADD = \ >+ $(PAM_LIBS) \ >+ $(CMOCKA_LIBS) \ >+ $(POPT_LIBS) \ >+ $(SSSD_LIBS) \ >+ $(CARES_LIBS) \ >+ $(KRB5_LIBS) \ >+ $(SSSD_INTERNAL_LTLIBS) \ >+ $(SYSTEMD_LOGIN_LIBS) \ >+ libsss_idmap.la \ >+ libsss_ldap_common.la \ >+ libsss_krb5_common.la \ >+ libsss_test_common.la >+ Compilation fails with LDFLAG --as-needed CCLD test_search_bases ./.libs/libsss_ldap_common.so: undefined reference to `is_domain_sid' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_bin_sid_to_sid' ./.libs/libsss_ldap_common.so: undefined reference to `idmap_error_string' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_autorid' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_rangesize' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_domain_by_name_has_algorithmic_mapping' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_free_sid' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_init' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_sid_to_unix' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_upper' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_calculate_range' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_set_lower' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_add_domain_ex' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_ctx_get_upper' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_unix_to_sid' ./.libs/libsss_ldap_common.so: undefined reference to `sss_idmap_domain_has_algorithmic_mapping' clang: error: linker command failed with exit code 1 (use -v to see invocation) make[2]: *** [test_search_bases] Error 1 make[2]: Leaving directory `/dev/shm/sssd_build' make[1]: *** [check-am] Error 2 make[1]: Leaving directory `/dev/shm/sssd_build' make: *** [check-recursive] Error 1 Please change order of libraries. diff --git a/Makefile.am b/Makefile.am index 239a248..7fdb315 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1409,8 +1409,8 @@ test_search_bases_LDADD = \ $(KRB5_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \ $(SYSTEMD_LOGIN_LIBS) \ - libsss_idmap.la \ libsss_ldap_common.la \ + libsss_idmap.la \ libsss_krb5_common.la \ libsss_test_common.la LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
