[SSSD] Behaviour of getgrnam/getgrgid

Ralf Haferkamp rhafer at suse.de
Fri Sep 24 09:27:55 UTC 2010


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



More information about the sssd-devel mailing list