-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
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.
- -- 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 SIGNED MESSAGE----- Hash: SHA1
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
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 */
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.
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.
-----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 SIGNED MESSAGE----- Hash: SHA1
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
-----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/
sssd-devel@lists.fedorahosted.org