On Thu, 2013-11-28 at 10:31 +0100, Lukas Slebodnik wrote:
> 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.cse(a)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.reichl(a)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(a)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.reichl(a)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.reichl(a)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(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Patches addressing both Jakub's and Lukas's suggestions are attached.
My style comment was addressed so ACK from me. I haven't tested if the
-as-needed linker issue was fixed, I'll let Lukas do that.