-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 07/29/2010 07:32 AM, Jakub Hrozek wrote:
On 07/28/2010 09:29 PM, Stephen Gallagher wrote:
> Patch 0002: Add a utility function to compare two non-ordered lists of
> strings. It stores the first list as keys in a dhash table, then
> attempts to remove all the keys in the second list with dhash_remove().
> If the return value is success, that value was in both, if it's not
> found, then it was only in the second list. The leftover keys at the end
> are returned as the values only in the first list.
Just one comment:
+ if (!_list1) {
+ list1 = talloc_array(tmp_ctx, char *, 1);
+ if (!list1) {
+ talloc_free(tmp_ctx);
+ return ENOMEM;
+ }
+ list1[0] = NULL;
+ }
+ else {
+ list1 = _list1;
+ }
Since we are duplicating the arrays later on with talloc_strdup, I think
it would be OK to just have something along the lines:
char *sentinel[] = { NULL };
list1 = _list1 ? _list1 : sentinel;
I didn't want to do this because I fully expect this function to be used
elsewhere and I want to guarantee that the memory heirarchy is safe in
that case.
> Patch 0003: Add sysdb_group_dn_reverse()
> This is a simple utility to just return the rdn name portion of a
> distinguished name.
Ack to the code, but why is the function called sysdb_group_dn_reverse?
I couldn't come up with anything better. It does the reverse of
sysdb_group_dn(), which takes a domain and name and returns an ldb_dn.
This takes a dn and splits out the group name.
I'm not sure what name to give it that makes it more descriptive:
sysdb_group_dn_name() maybe?
> Patch 0004: Add a utility routine to duplicate a NULL-terminated
list of
> strings.
Ack
> Patch 0005: Add sysdb_update_members function
> This function will take a user, a list of groups that this user
> should be added to and a list of groups the user should be removed
> from and will recursively call sysdb_[add|remove]_group_member
Ack, looks OK, perhaps it would be nice to have a unit test
Yeah, I was planning to write one at some point, but I was more focused
on getting the code working. I will add it today along with my other fixes.
> Patch 0006: Clean up initgroups processing for RFC2307
> Instead of recursively updating all users of each group the user
> being queried belongs to, just add or remove membership for the
> requested user.
The last parameter of sdap_initgr_rfc2307_send now looks redundant.
Ah right. I meant to remove that.
Maybe it would be nice to move the allocation of attributes one level
up
and pass it via the grp_attrs param?
No, I think this would be confusing, since the grp_attrs param is used
for the other schemas as well.
Nitpick:
+ }
+
+ else {
The extra blank like here looks confusing.
Sure.
- --
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 v2.0.14 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkxRaRgACgkQeiVVYja6o6OiEwCfVCB2H3I5K0fZib8lh0qukOX+
OfwAoJuzCHivV/irQWVuDqNBoStaMyO9
=ohDe
-----END PGP SIGNATURE-----