On Wed 14 Nov 2012 06:39:06 PM EST, Paul B. Henson wrote:
On 11/14/2012 1:41 PM, Stephen Gallagher wrote:
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.
Done.
../src/providers/ldap/sdap.h:481:5: note: expected 'const char **' but argument is of type 'char **'
Fixed.
You need to add 'ignore_group_members' to the tuple of expected values in the test in SSSDConfigTest.py
D'oh, I actually had an open editor buffer with that change in it that it seems I lost track of and never saved 8-/.
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.
Done.
!state->dom->ignore_group_members with a comment as to what that means? The if-then-else construct is a bit confusing there.
Good suggestion, I guess I had just had ternaries on my mind from the previous use :).
nsssrv_cmd.c has added a tab at the beginning of the line if (!dom->ignore_group_members) {
Oops, I try to play chameleon when working on other people's code bases but I guess my editor slipped that one past me.
I've attached an updated patch that I believe addresses all of your issues, let me know if I missed something or anything else needs a tuneup.
Thanks...
I've run some tests on this today. With the option unset or False, behavior appears to be properly unmodified from the existing code (Good). I ran a simple performance test on a moderately complex AD environment I already had set up.
That environment has 3000 users that are a members of a series of nested groups. I have the following baseline for performance (on a completely empty cache):
[root@sgallagh520 db]# time id kurashima@adtest uid=201145(kurashima) gid=200513(domain users) groups=200513(domain users),204697(abovetopgroup),201108(topgroup),201109(nestedgroup),204699(aboveeventhat),204695(uppergroup),201107(sssdgroup),204698(aboveabovetop)
real 0m1.795s user 0m0.002s sys 0m0.009s
Changing only the ignore_group_members output, this becomes:
[root@sgallagh520 db]# time id kurashima@adtest uid=201145(kurashima) gid=200513(domain users) groups=200513(domain users),204697(abovetopgroup),201108(topgroup),201109(nestedgroup),204699(aboveeventhat),204695(uppergroup),201107(sssdgroup),204698(aboveabovetop)
real 0m0.441s user 0m0.002s sys 0m0.007s
This is obviously already a significant enhancement, and of course the difference will be more pronounced for much larger environments. I'm prepared to give this an ack, with one comment to whoever pushes the patch upstream: please reflow the changes in nsssrv_cmd.c's fill_grent() that are now exceeding the 80-character line limit due to the new indentation.