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

Pavel Reichl preichl at redhat.com
Mon Dec 2 09:26:29 UTC 2013


On Sun, 2013-12-01 at 15:46 -0500, Dmitri Pal wrote:
> On 11/29/2013 09:33 AM, Jakub Hrozek wrote:
> > 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.
> 
> The reason I brought it up is because when I read the code i++ reads
> cleanly while any use of of ++i forces me to stop, pause and drill down
> into why this construct is used.
> But may be it is just a question pf preference and habit.
> In my practice, however, I have seen i++ used about 100 time more
> frequently than ++i.
> 
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> 
> 

I agree that in this context postfix notation is a way more common. The
reason I used prefix notation comes from my background in c++ where it
is a rule of thumb to use prefix notation if possible because of its
performance gain (although this is mainly true for complex types).

Well, next time I'll use postfix notation to conform with the rest of
the code base.





More information about the sssd-devel mailing list