On Wed, Nov 27, 2013 at 03:24:44PM +0100, 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
Looks good and works fine.
I will just squash in two small style changes I didn't tell you about
earlier, but no need to resend the patches:
--- a/src/tests/cmocka/test_search_bases.c
+++ b/src/tests/cmocka/test_search_bases.c
@@ -53,7 +53,7 @@ static struct sdap_search_base** generate_bases(TALLOC_CTX *mem_ctx,
search_bases = talloc_array(mem_ctx, struct sdap_search_base *, n + 1);
assert_non_null(search_bases);
- for(i=0; i < n; ++i) {
+ 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) {
@@ -135,7 +135,7 @@ static void do_test_get_by_dn(const char *dn, const char **dns, size_t
n,
opts->sdom = sdom;
res_sdom = sdap_domain_get_by_dn(opts, dn);
- switch(expected_result) {
+ switch (expected_result) {
case DN_NOT_IN_DOMS:
assert_null(res_sdom);
break;
We tend to use a whitespace after most keywords.
ACK.