[SSSD] [PATCHES] Fix severe performance problems with initgroups (master)

Stephen Gallagher sgallagh at redhat.com
Tue Aug 3 17:12:38 UTC 2010


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/03/2010 12:34 PM, Jakub Hrozek wrote:
> On 08/02/2010 05:58 PM, Stephen Gallagher wrote:
>> Attached are the patches to fix the initgroups performance issue for the
>> master branch. Patches 0001-0004 applied trivially to the master,
>> patches 0005 and 0006 had to be rewritten to take advantage of the
>> synchronous sysdb interface, but this has made them distinctly easier to
>> follow.
> 
> 
> 0001 to 0004 - no changes in the actual code (verified with
> vimdiff/interdiff) -> ACK

Coverity noticed a small (but distinct) mistake in patch 3.
We're doing a NULL check for the wrong variable after the
talloc_strdup(). This is now fixed and will be backported.

> 
> 0005 - works OK, but there's a change of behavior vs. the 1.2 version. I
> actually prefer the new one, so I'm inclined to ACK it, but I think it
> is worth at least pointing out. The difference is that the 1.2 code
> would abort the tevent request in case sysdb_add_group_member_recv() or
> sysdb_remove_group_member_recv() failed with tevent_req_error(). The new
> version just skips the errors:
> 
> +            ret = sysdb_add_group_member(tmp_ctx, sysdb, domain,
> +                                         add_groups[i], user);
> +            if (ret != EOK) {
> +                DEBUG(1, ("Could not add user [%s] to group [%s]. "
> +                          "Skipping.\n"));
> +                /* Continue on, we should try to finish the rest */
> 

Yeah, I rethought how best to handle this and decided that it was safer
to skip and try any of the others rather than to throw an error. I think
I may go back and make this change to 1.2 as well.

> Also, there is a minor difference in the interface as the new one
> accepts "const char *" the old one did take "char *"..looking at the
> explicit cast in patch 0006 that's probably intentional.

Yeah, I wanted the interface to be clearer. I'll make the same change to
1.2 if I fix the above.

> 
> 0006 - just two nitpicks: no space after if:
> 
>  +    if(!state->name) {
> 
> and "else" on separate line:
> 
> +    }
> +    else {
> 
> 
> Also, the 1.2 patch made the sdap_initgr_rfc2307_send properly "static",
> might be nice to do that here, too.
> 
> Otherwise, looks OK.

Fixed both nitpicks. New patch attached.

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

iEYEARECAAYFAkxYTgYACgkQeiVVYja6o6N7QQCffm1BjYLi4Ch9m2KiB+YD+F4J
zBcAni5P676iiRPM7Uu+AhJ+rRTlU25V
=l7yT
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-sysdb_group_dn_name-utility-function.patch
Type: text/x-patch
Size: 2102 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100803/8f55ef5b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Clean-up-initgroups-processing-for-RFC2307.patch
Type: text/x-patch
Size: 7006 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100803/8f55ef5b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-sysdb_group_dn_name-utility-function.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100803/8f55ef5b/attachment.sig>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Clean-up-initgroups-processing-for-RFC2307.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100803/8f55ef5b/attachment-0001.sig>


More information about the sssd-devel mailing list