-----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-----