[SSSD] [PATCHES] Fix severe performance problems with initgroups

Stephen Gallagher sgallagh at redhat.com
Thu Jul 29 11:42:16 UTC 2010


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



More information about the sssd-devel mailing list