[SSSD] [PATCH] - Improved detection of user domain

Jakub Hrozek jhrozek at redhat.com
Tue Nov 26 17:25:02 UTC 2013


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 at 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 at 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.



More information about the sssd-devel mailing list