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.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
