-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 09/20/2010 11:13 AM, Ralf Haferkamp wrote: > On Friday 17 September 2010 19:56:15 Stephen Gallagher wrote: >> On 09/17/2010 12:16 PM, Ralf Haferkamp wrote: >>> Find a new version attached. Does that look better? If that is not >>> what you were referring to lets discuss it in IRC on monday. >>> >>> Note, I needed to implement sdap_process_group_send() slightly >>> differnent than the other _send functions in sdap_async_accounts(). >>> Instead of returning as tevent_req* it does return a error code. >>> The tevent_req* is returned via the argument list. I did this >>> because there are cases where sdap_process_group_send() does not >>> need to create a new tevent_req*, e.g. when the groups doesn't have >>> any members or when all groupmembers are already cached. >> >> In situations like this, the correct behavior is still to return the >> request, but inside the _send() function call: >> >> tevent_req_done(req); >> tevent_req_post(req, ev); >> return req; >> >> This way it will just call the finish handler immediately after the >> parent function returns. Then you don't need to have separate >> codepaths for functions that do or do not have to make subrequests. > 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. 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. 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]. 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. 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'. - -- Stephen Gallagher RHCE 804006346421761 Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAkyblisACgkQeiVVYja6o6PODACcDBQ414veIaUJ8w2pV1P2c5JP WzcAoJGrMVPm7I7hC783z8JIcLWupLXl =TESR -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
