On Thursday 23 September 2010 20:02:20 Stephen Gallagher wrote:
On 09/20/2010 11:13 AM, Ralf Haferkamp wrote:
[..]
> Nice, that makes the code a little cleaner, thanks. New patches
> attached.
Patch 0001: Ack. This looks fine to me.
Patch 0002: Nack.
There are still a few style issues.
Please move the struct sdap_process_group_state definition to right
before the sdap_get_groups_process() implementation.
will do
I'm not a huge fan of firing off all of the
sdap_process_group_state()
calls in a loop for (effectively) simultaneous operation. I'd prefer
if you implemented this with our _step() approach (where you handle
each one serially until they're all finished). It's quite a bit
easier to follow in a debugger. This will also eliminate the need for
GROUPMEMBER_REQ_PARALLEL. Believe it or not, it won't be any slower
because the LDAP id op operations will serialize the requests anyway
so we don't open large numbers of separate connections to the LDAP
server.
I agree that the parallel approach is a bit more complex. But it was
significantly faster in the tests I did compared to serially processing
the group members. Otherwise I wouldn't have done that implementation.
BTW, you can see the difference by setting GROUPMEMBER_REQ_PARALLEL to 0.
The goal wasn't to have multiple LDAP connections opened (that would be
insane). But sending multiple request at once to the LDAP server (through
a single connection) and then starting processing the results of the
requests is of course faster than sending sending a single request then
processing the result of that request and then sending the next request.
Simply because the LDAP server can a) process multiple requests at once
and b) has a some request queued to process while the client is busy with
retrieving and processing the results. The only drawback I see here is
that the LDAP Server might limit the number of LDAP request it queues on
a single connection therefor it would probably be good to replace
GROUPMEMBER_REQ_PARALLEL with a value configurable via sssd.conf.
There's a memory hierarchy bug in sdap_process_group_send(). You
allocate sysdb_dn on memctx, but it really should be a child of
grp_state->sysdb_dns->values[grp_state->sysdb_dns->num_values].
Ok,
I'll check that.
I think there's a lot of potential here. We just need to work on
cleaning things up a little bit further.
Patch 0003: Nack.
I agree with Simo that it would make more sense to use a nesting level
rather than a boolean for this task.
Yeah, I fine with taking Simo's approach
here.
Also, any changes to the LDAP options need to be added to
src/config/etc/sssd.api.d/sssd-ldap.conf and have a translatable
string added to src/config/SSSDConfig.py and also have the same
option mirrored in the IPA backend. This latter you will be alerted
to if you attempt to run 'make check'.
--
Ralf