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

Jakub Hrozek jhrozek at redhat.com
Fri Nov 29 14:33:02 UTC 2013


On Fri, Nov 29, 2013 at 01:34:27AM -0500, Dmitri Pal wrote:
> On 11/28/2013 03:52 AM, Jakub Hrozek wrote:
> > 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 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.
> >>> _______________________________________________
> >>> sssd-devel mailing list
> >>> sssd-devel at 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) {
> 
> I hate to rain on parade but ++i is really confusing.
> I know that for "for" loops it makes no difference (at least this is
> what Goggle said) but for me any use of ++i seems odd.
> It is "increment then do" with languages like C that is 0 based for
> arrays it is more "do and then increment".
> Does the use of ++i (though legitimate in this case) makes only me
> uncomfortable or I am not the only one?
> Can we use i++ and reserve ++i only for the cases where it is really
> meaningful and justified?
> I do not see anything in the style guide about this but should we have
> something?

I'm personally indifferent to this, although I prefer the postfix
notation (i++) as well. But that's just my personal preference.



More information about the sssd-devel mailing list