Patch 0001: Added a debug message to see which record type we're processing on each loop through sdap_process_message(). This is purely informational.
Patch 0002: Add support for paged LDAP results. I changed the internals of sdap_get_generic_send() somewhat here and added a new sdap_get_generic_internal() routine that can be used to handle multiple calls to LDAP for a single request. We should be able to use this in the future to handle Active Directory's non-standard attribute-level paging as well (though that's not addressed in this patch).
Patch 0003: Add ldap_page_size configuration option I made this a separate patch for simplicity of review. I set 1000 records as the default, as this seemed to be the most-compatible value among 389, OpenLDAP and ActiveDirectory as best I could determine.
On Mon, 2011-04-25 at 14:26 -0400, Stephen Gallagher wrote:
Patch 0001: Added a debug message to see which record type we're processing on each loop through sdap_process_message(). This is purely informational.
Patch 0002: Add support for paged LDAP results. I changed the internals of sdap_get_generic_send() somewhat here and added a new sdap_get_generic_internal() routine that can be used to handle multiple calls to LDAP for a single request. We should be able to use this in the future to handle Active Directory's non-standard attribute-level paging as well (though that's not addressed in this patch).
Patch 0003: Add ldap_page_size configuration option I made this a separate patch for simplicity of review. I set 1000 records as the default, as this seemed to be the most-compatible value among 389, OpenLDAP and ActiveDirectory as best I could determine.
Simo made some comments on IRC that I have incorporated.
First, I renamed sdap_get_generic_send_internal() to sdap_get_generic_step() and reordered it according to the tevent_req style. Second, I added talloc_zfree(state->op) to the beginning of sdap_get_generic_step() to ensure that we aren't carrying around extra sdap_op memory for subsequent pages.
On Mon, 2011-04-25 at 15:33 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 14:26 -0400, Stephen Gallagher wrote:
Patch 0001: Added a debug message to see which record type we're processing on each loop through sdap_process_message(). This is purely informational.
Patch 0002: Add support for paged LDAP results. I changed the internals of sdap_get_generic_send() somewhat here and added a new sdap_get_generic_internal() routine that can be used to handle multiple calls to LDAP for a single request. We should be able to use this in the future to handle Active Directory's non-standard attribute-level paging as well (though that's not addressed in this patch).
Patch 0003: Add ldap_page_size configuration option I made this a separate patch for simplicity of review. I set 1000 records as the default, as this seemed to be the most-compatible value among 389, OpenLDAP and ActiveDirectory as best I could determine.
Simo made some comments on IRC that I have incorporated.
First, I renamed sdap_get_generic_send_internal() to sdap_get_generic_step() and reordered it according to the tevent_req style. Second, I added talloc_zfree(state->op) to the beginning of sdap_get_generic_step() to ensure that we aren't carrying around extra sdap_op memory for subsequent pages.
Simo had a few remaining comments. To follow tevent_req style, I've moved forward declarations to be above the sdap_get_generic_send().
Also, there was a memory issue (I was accessing memory that had been leaked). The cookie had gone out of scope, but I hadn't freed the memory (which is why I saw no issues in my tests).
I now make an appropriate copy of the cookie berval and free the original memory.
On Mon, 2011-04-25 at 16:21 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 15:33 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 14:26 -0400, Stephen Gallagher wrote:
Patch 0001: Added a debug message to see which record type we're processing on each loop through sdap_process_message(). This is purely informational.
Patch 0002: Add support for paged LDAP results. I changed the internals of sdap_get_generic_send() somewhat here and added a new sdap_get_generic_internal() routine that can be used to handle multiple calls to LDAP for a single request. We should be able to use this in the future to handle Active Directory's non-standard attribute-level paging as well (though that's not addressed in this patch).
Patch 0003: Add ldap_page_size configuration option I made this a separate patch for simplicity of review. I set 1000 records as the default, as this seemed to be the most-compatible value among 389, OpenLDAP and ActiveDirectory as best I could determine.
Simo made some comments on IRC that I have incorporated.
First, I renamed sdap_get_generic_send_internal() to sdap_get_generic_step() and reordered it according to the tevent_req style. Second, I added talloc_zfree(state->op) to the beginning of sdap_get_generic_step() to ensure that we aren't carrying around extra sdap_op memory for subsequent pages.
Simo had a few remaining comments. To follow tevent_req style, I've moved forward declarations to be above the sdap_get_generic_send().
Also, there was a memory issue (I was accessing memory that had been leaked). The cookie had gone out of scope, but I hadn't freed the memory (which is why I saw no issues in my tests).
I now make an appropriate copy of the cookie berval and free the original memory.
One more change: the cookie is now copied with talloc_memdup() instead of talloc_strdup(), since it may not be a proper string (I'm looking at YOU Active Directory).
On Mon, 2011-04-25 at 16:41 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 16:21 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 15:33 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 14:26 -0400, Stephen Gallagher wrote:
Patch 0001: Added a debug message to see which record type we're processing on each loop through sdap_process_message(). This is purely informational.
Patch 0002: Add support for paged LDAP results. I changed the internals of sdap_get_generic_send() somewhat here and added a new sdap_get_generic_internal() routine that can be used to handle multiple calls to LDAP for a single request. We should be able to use this in the future to handle Active Directory's non-standard attribute-level paging as well (though that's not addressed in this patch).
Patch 0003: Add ldap_page_size configuration option I made this a separate patch for simplicity of review. I set 1000 records as the default, as this seemed to be the most-compatible value among 389, OpenLDAP and ActiveDirectory as best I could determine.
Simo made some comments on IRC that I have incorporated.
First, I renamed sdap_get_generic_send_internal() to sdap_get_generic_step() and reordered it according to the tevent_req style. Second, I added talloc_zfree(state->op) to the beginning of sdap_get_generic_step() to ensure that we aren't carrying around extra sdap_op memory for subsequent pages.
Simo had a few remaining comments. To follow tevent_req style, I've moved forward declarations to be above the sdap_get_generic_send().
Also, there was a memory issue (I was accessing memory that had been leaked). The cookie had gone out of scope, but I hadn't freed the memory (which is why I saw no issues in my tests).
I now make an appropriate copy of the cookie berval and free the original memory.
One more change: the cookie is now copied with talloc_memdup() instead of talloc_strdup(), since it may not be a proper string (I'm looking at YOU Active Directory).
Ack, although it would be nice to have someone else test it before actually pushing to master.
Simo.
On 04/25/2011 10:52 PM, Simo Sorce wrote:
On Mon, 2011-04-25 at 16:41 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 16:21 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 15:33 -0400, Stephen Gallagher wrote:
On Mon, 2011-04-25 at 14:26 -0400, Stephen Gallagher wrote:
Patch 0001: Added a debug message to see which record type we're processing on each loop through sdap_process_message(). This is purely informational.
Patch 0002: Add support for paged LDAP results. I changed the internals of sdap_get_generic_send() somewhat here and added a new sdap_get_generic_internal() routine that can be used to handle multiple calls to LDAP for a single request. We should be able to use this in the future to handle Active Directory's non-standard attribute-level paging as well (though that's not addressed in this patch).
Patch 0003: Add ldap_page_size configuration option I made this a separate patch for simplicity of review. I set 1000 records as the default, as this seemed to be the most-compatible value among 389, OpenLDAP and ActiveDirectory as best I could determine.
Simo made some comments on IRC that I have incorporated.
First, I renamed sdap_get_generic_send_internal() to sdap_get_generic_step() and reordered it according to the tevent_req style. Second, I added talloc_zfree(state->op) to the beginning of sdap_get_generic_step() to ensure that we aren't carrying around extra sdap_op memory for subsequent pages.
Simo had a few remaining comments. To follow tevent_req style, I've moved forward declarations to be above the sdap_get_generic_send().
Also, there was a memory issue (I was accessing memory that had been leaked). The cookie had gone out of scope, but I hadn't freed the memory (which is why I saw no issues in my tests).
I now make an appropriate copy of the cookie berval and free the original memory.
One more change: the cookie is now copied with talloc_memdup() instead of talloc_strdup(), since it may not be a proper string (I'm looking at YOU Active Directory).
Ack, although it would be nice to have someone else test it before actually pushing to master.
I tested it against our corporate LDAP server plus my devel IPA server. Seems to work just fine.
I have two comments, though - ldap_create_page_control() and ldap_parse_pageresponse_control() don't seem to exist on RHEL5. We should probably #ifdef this functionality out (or implement it for old libldap releases).
Also the "paging_criticality" variable is char, but the iscritical parameter of ldap_parse_pageresponse_control() is declared as int.
The rest looks good to me.
On Tue, 2011-04-26 at 17:37 +0200, Jakub Hrozek wrote:
I tested it against our corporate LDAP server plus my devel IPA server. Seems to work just fine.
I have two comments, though - ldap_create_page_control() and ldap_parse_pageresponse_control() don't seem to exist on RHEL5. We should probably #ifdef this functionality out (or implement it for old libldap releases).
Also the "paging_criticality" variable is char, but the iscritical parameter of ldap_parse_pageresponse_control() is declared as int.
The rest looks good to me.
Thanks for catching the paging_criticality bug. That was clearly wrong. I was working from some old IBM documentation that had it as a char. With the way I had it implemented before, it was incorrectly set as critical.
As for the comment about RHEL5, this is not an issue. RHEL 5.7 and later is shipping with openldap 2.4.24 (as an openldap24-libs RPM) that SSSD is linked against.
I'll open a bug against it, but I don't think it's urgent to fix it at present.
On 04/26/2011 07:16 PM, Stephen Gallagher wrote:
On Tue, 2011-04-26 at 17:37 +0200, Jakub Hrozek wrote:
I tested it against our corporate LDAP server plus my devel IPA server. Seems to work just fine.
I have two comments, though - ldap_create_page_control() and ldap_parse_pageresponse_control() don't seem to exist on RHEL5. We should probably #ifdef this functionality out (or implement it for old libldap releases).
Also the "paging_criticality" variable is char, but the iscritical parameter of ldap_parse_pageresponse_control() is declared as int.
The rest looks good to me.
Thanks for catching the paging_criticality bug. That was clearly wrong. I was working from some old IBM documentation that had it as a char. With the way I had it implemented before, it was incorrectly set as critical.
As for the comment about RHEL5, this is not an issue. RHEL 5.7 and later is shipping with openldap 2.4.24 (as an openldap24-libs RPM) that SSSD is linked against.
I'll open a bug against it, but I don't think it's urgent to fix it at present.
OK, I wasn't aware of this and I'm fine with this approach (and I'm glad I haven't ported back the deref parsing yet :-)).
Ack to all three patches.
On Wed, 2011-04-27 at 10:19 +0200, Jakub Hrozek wrote:
On 04/26/2011 07:16 PM, Stephen Gallagher wrote:
On Tue, 2011-04-26 at 17:37 +0200, Jakub Hrozek wrote:
I tested it against our corporate LDAP server plus my devel IPA server. Seems to work just fine.
I have two comments, though - ldap_create_page_control() and ldap_parse_pageresponse_control() don't seem to exist on RHEL5. We should probably #ifdef this functionality out (or implement it for old libldap releases).
Also the "paging_criticality" variable is char, but the iscritical parameter of ldap_parse_pageresponse_control() is declared as int.
The rest looks good to me.
Thanks for catching the paging_criticality bug. That was clearly wrong. I was working from some old IBM documentation that had it as a char. With the way I had it implemented before, it was incorrectly set as critical.
As for the comment about RHEL5, this is not an issue. RHEL 5.7 and later is shipping with openldap 2.4.24 (as an openldap24-libs RPM) that SSSD is linked against.
I'll open a bug against it, but I don't think it's urgent to fix it at present.
OK, I wasn't aware of this and I'm fine with this approach (and I'm glad I haven't ported back the deref parsing yet :-)).
Ack to all three patches.
Pushed to master
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Stephen Gallagher