On Wed 14 Nov 2012 02:28:07 PM EST, Simo Sorce wrote:
On Wed, 2012-11-14 at 10:59 -0800, Paul B. Henson wrote:
> On Wed, Nov 14, 2012 at 09:04:24AM -0500, Simo Sorce wrote:
>
>> Ok, put down this way it tips my opinion toward the currently proposed
>> patch.
>
> Cool. Is there anything else I need to do to move this forward?
No we just need someone to test and ack your patch.
Simo.
First of all, thank you very much for the contribution. It's a great
start. I've got a few bits and pieces for you to clean up before we
commit it upstream.
Minor: Please use the new SSSDBG macros in confdb_get_domain_internal().
You don't need to update the existing code, but all new code should use
the macros. See util.h for a listing of them.
Build issue:
../src/providers/ldap/ldap_id.c: In function 'groups_get_send':
../src/providers/ldap/ldap_id.c:448:32: error: passing argument 4 of
'build_attrs_from_map' from incompatible pointer type [-Werror]
In file included from ../src/providers/ldap/ldap_common.h:26:0,its o
from ../src/providers/ldap/ldap_id.c:32:
../src/providers/ldap/sdap.h:481:5: note: expected 'const char **' but
argument is of type 'char **'
cc1: all warnings being treated as errors
I recommend building with:
make CFLAGS+="-ggdb3 -O0 -Wall -Wextra -Wno-unused-parameter
-Wno-sign-compare -Wformat-security -Werror"
Test issue:
.....F...F...
======================================================================
FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain)
----------------------------------------------------------------------
Traceback (most recent call last):
File "./../src/config/SSSDConfigTest.py", line 539, in testListOptions
option)
AssertionError: Option [ignore_group_members] unexpectedly found
======================================================================
FAIL: testRemoveProvider (__main__.SSSDConfigTestSSSDDomain)
----------------------------------------------------------------------
Traceback (most recent call last):
File "./../src/config/SSSDConfigTest.py", line 879, in testRemoveProvider
option)
AssertionError: Option [ignore_group_members] unexpectedly found
----------------------------------------------------------------------
Ran 13 tests in 0.094s
FAILED (failures=2)
You need to add 'ignore_group_members' to the tuple of expected values
in the test in SSSDConfigTest.py
Style issue:
/* TODO: handle attrs_type */
ret = build_attrs_from_map(state, ctx->opts->group_map,
SDAP_OPTS_GROUP,
- NULL, &state->attrs, NULL);
+ state->domain->ignore_group_members ?
+ member_filter : NULL, &state->attrs, NULL);
Please indent the rest of the ?: structure so that it's clear that
member_filter : NULL is part of the above. &state->attrs, NULL should be
on the next line. It'll read easier that way.
In sdap_get_groups_done(), wouldn't it just be easier to use
!state->dom->ignore_group_members with a comment as to what that means?
The if-then-else construct is a bit confusing there.
nsssrv_cmd.c has added a tab at the beginning of the line
if (!dom->ignore_group_members) {
SSSD is standardized on four-space indentation. (And my editor lights it
up like neon when tabs are added)
Other than that, the code looks good. I haven't had a chance to test it
yet, but I'll try to get to that in the next two days.