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....@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.rei...@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@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?
> 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.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
