[SSSD] [PATCHES] Fix severe performance problems with initgroups (master)
Stephen Gallagher
sgallagh at redhat.com
Tue Aug 3 17:57:08 UTC 2010
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/03/2010 01:34 PM, Jakub Hrozek wrote:
> On 08/03/2010 07:12 PM, Stephen Gallagher wrote:
>> 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.
>
>
> Obviously correct, ack
>
>
>>> 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.
>
>
> Ack
Pushed all six patches to master.
- --
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/
iEYEARECAAYFAkxYWHQACgkQeiVVYja6o6PS/gCfYKqhZUWYF0x9kmRMjY50vgM5
o6MAn2HmLh6CFvHB12SnWaic9gdvv842
=AN8W
-----END PGP SIGNATURE-----
More information about the sssd-devel
mailing list