Full_Name: Hans Leidekker Version: CVS OS: Linux URL: http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch Submission from: (NULL) (84.107.25.189) OpenLDAP does not provide any C API calls to encode/parse paged results requests/responses (RFC2696), although it supports the page control in the server. Augmenting the C API with such calls would fix this imbalance and also make porting certain client software to OpenLDAP easier. So here's a proposed a patch that implements the ldap_create_page_control and ldap_parse_page_control APIs: http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch
On Sun, 2006-01-08 at 15:58 +0000, hans@it.vu.nl wrote: > OpenLDAP does not provide any C API calls to encode/parse paged results > requests/responses (RFC2696), although it supports the page control in > the server. Augmenting the C API with such calls would fix this imbalance > and also make porting certain client software to OpenLDAP easier. > > So here's a proposed a patch that implements the ldap_create_page_control > and ldap_parse_page_control APIs: > > http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch Hans, thanks for the contribution. I have essentially one comment, which applies to your contribution as well as to many other implementations of controls spread into OpenLDAP client library code (and, I guess, in other implementations of the client library). The "tough" part of creating or parsing a control is encoding/decoding the control value, if any; what we really need is hiding the details of value encoding/decoding behind a clear interface that takes/returns the high level info related to the control (the same applies to extended operations). The API you propose appears to be based on that of the vlv control, which is unnecessarily "pedantic" and wastes a lot of memory in unnecessary memory allocation both ways. The typical use of server controls is with the ldap_*_ext* functions; in my practice I usually write code like: LDAPControl c, *ctrls[2]; c.ldctl_oid = LDAP_CONTROL_*; c.ldctl_iscritical = iscritical; c.ldctl_value = /* the tough part */ ; ctrls[0] = &c; ctrls[1] = NULL; ldap_*_ext(ld, ..., ctrls, ...); Similarly, when parsing results: ldap_parse_result(ld, msg, ..., &ctrls, freeme); if (ctrls != NULL) { for (i = 0; ctrls[i] != NULL; i++) { if (strcmp(ctrls[i]->ldctl_oid, LDAP_CONTROL_*) == 0) { handle( &ctrls[i]->ldctl_value ); /* the tough part */ } else { /* ??? */ } } } So I think your API is fine for consistency with other controls, but I'd prefer to split it in two levels: "value" level: deal with encoding/decoding the value; something like int ldap_create_page_control_value( int pagesize, /* in */ struct berval *cookie, /* in */ struct berval *value ); /* out; only malloc() bv_val */ int ldap_parse_page_control_value( struct berval *value, /* in */ int *estimate, /* out */ struct berval *cookie); /* out; only malloc() bv_val */ These two functions can be used by "smart" code to eliminate the redundant part (OID and LDAPControl allocation in create; OID lookup in parse; and so on). The calls you propose could still be present for consistency with the current API; they can use the "smart" calls to handle the ldctl_value. On a related note, the cookie is supposed to be __empty__ as per RFC 2696 either when sent the first time or when returned the last time, so I don't see any reason to have ldap_parse_page_control() pass a "struct berval **" for it, since that implies that also the "struct berval" has to be malloc()'ed; simply pass a "struct berval *" and make the function set bv_len = 0 when the cookie is empty. Comments? p. Ing. Pierangelo Masarati Responsabile Open Solution OpenLDAP Core Team SysNet s.n.c. Via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ------------------------------------------ Office: +39.02.23998309 Mobile: +39.333.4963172 Email: pierangelo.masarati@sys-net.it ------------------------------------------
changed notes moved from Incoming to Contrib
I would like to this be reworked using ppolicy API as a basis than the older sort/vlv control APIs (which should also be reworked). Parse routines shouldn't also find the control as this makes it harder to use in programs that include their own finding. Kurt At 07:58 AM 1/8/2006, hans@it.vu.nl wrote: >Full_Name: Hans Leidekker >Version: CVS >OS: Linux >URL: http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch >Submission from: (NULL) (84.107.25.189) > > > >OpenLDAP does not provide any C API calls to encode/parse paged results >requests/responses (RFC2696), although it supports the page control in >the server. Augmenting the C API with such calls would fix this imbalance >and also make porting certain client software to OpenLDAP easier. > >So here's a proposed a patch that implements the ldap_create_page_control >and ldap_parse_page_control APIs: > > http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch
On Sun, 2006-01-08 at 18:26 +0000, Kurt@OpenLDAP.org wrote: > I would like to this be reworked using ppolicy API as a basis than > the older sort/vlv control APIs (which should also be reworked). > Parse routines shouldn't also find the control as this makes it > harder to use in programs that include their own finding. I think ppolicy API also suffers from some excessive malloc()'ing in the create function. I agree for the rest. p. Ing. Pierangelo Masarati Responsabile Open Solution OpenLDAP Core Team SysNet s.n.c. Via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ------------------------------------------ Office: +39.02.23998309 Mobile: +39.333.4963172 Email: pierangelo.masarati@sys-net.it ------------------------------------------
At 10:48 AM 1/8/2006, ando@sys-net.it wrote: >I think ppolicy API also suffers from some excessive malloc()'ing in the >create function. I agree for the rest. Feel free to update it... and, given I just removed the finding from sort/vlv parsers, feel free to make appropriate changes there as well.
On Sunday 08 January 2006 19:12, Pierangelo Masarati wrote: > So I think your API is fine for consistency with other controls, but I'd > prefer to split it in two levels: Sure, I can split things up like that. > On a related note, the cookie is supposed to be __empty__ as per RFC > 2696 either when sent the first time or when returned the last time, so > I don't see any reason to have ldap_parse_page_control() pass a "struct > berval **" for it, since that implies that also the "struct berval" has > to be malloc()'ed; simply pass a "struct berval *" and make the function > set bv_len = 0 when the cookie is empty. Good point. On the other hand, others have set the trend already: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/ldap/ldap/ldap_parse_page_control.asp http://www-03.ibm.com/servers/eserver/iseries/ldap/ldap_parse_page_control.htm If we do deviate from those examples I think we should also change the "const char" parameter for specifying criticality into an "int", for consistency with ldap_create_sort_control() and ldap_create_control(). Oh, and thank you for your elaborate comments. -Hans
On Sun, 2006-01-08 at 20:47 +0100, Hans Leidekker wrote: > On Sunday 08 January 2006 19:12, Pierangelo Masarati wrote: > > > So I think your API is fine for consistency with other controls, but I'd > > prefer to split it in two levels: > > Sure, I can split things up like that. It's ok, I've already split things (as I was doing the same for vlv, sort & so). > > > On a related note, the cookie is supposed to be __empty__ as per RFC > > 2696 either when sent the first time or when returned the last time, so > > I don't see any reason to have ldap_parse_page_control() pass a "struct > > berval **" for it, since that implies that also the "struct berval" has > > to be malloc()'ed; simply pass a "struct berval *" and make the function > > set bv_len = 0 when the cookie is empty. > > Good point. On the other hand, others have set the trend already: > > http://msdn.microsoft.com/library/default.asp?url=/library/en-us/ldap/ldap/ldap_parse_page_control.asp > http://www-03.ibm.com/servers/eserver/iseries/ldap/ldap_parse_page_control.htm > > If we do deviate from those examples I think we should also change the > "const char" parameter for specifying criticality into an "int", for > consistency with ldap_create_sort_control() and ldap_create_control(). I see. For create, I'd prefer to stick with the convention of existing functions, although the ldctl_iscritical is actually a char so the existing convention is a bit irrational; in any case, I'm positive it's not going to bring in too much inconsistency, since automatic cast from char to int and viceversa should work relatively fine. For parse functions, there wouldn't be any issue: we can keep the consolidated form as is, and use ldap_parse_pageresult_control() for our API. I'm about to commit a rewiewed version of your patch and of the related cleanup of existing controls which follows Kurt's changes. p. Ing. Pierangelo Masarati Responsabile Open Solution OpenLDAP Core Team SysNet s.n.c. Via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ------------------------------------------ Office: +39.02.23998309 Mobile: +39.333.4963172 Email: pierangelo.masarati@sys-net.it ------------------------------------------
changed notes changed state Open to Test
changed notes moved from Contrib to Software Enhancements
On Sunday 08 January 2006 19:16, Kurt D. Zeilenga wrote: > I would like to this be reworked using ppolicy API as a basis than Can you specify what you mean by "ppolicy API"? I presume you mean taking a single control instead of an array?. And assert()ing on faulty parameters maybe? Anything else? I see a function ldap_parse_passwordpolicy_control() in ppolicy.c that does inline berval parsing, which contrasts with what Pierangelo said about splitting it up. > the older sort/vlv control APIs (which should also be reworked). > Parse routines shouldn't also find the control as this makes it > harder to use in programs that include their own finding. Hmm, anything else? ;-) So if I take into account what Pierangelo and you wrote I arrive at the following API: int ldap_create_page_control( LDAP *ld, unsigned long pagesize, struct berval *cookie, int iscritical, LDAPControl **ctrlp ); int ldap_parse_page_control ( LDAP *ld, LDAPControl *ctrls, unsigned long *count, struct berval *cookie ); Changes relative to the previous version: - change iscritical from a "const char" to an "int". - accept an "LDAPControl *" instead of an "LDAPControl **" - return a "struct berval *" instead of a "struct berval **". -Hans
I've committed something that looks like (ldap.h) LDAP_F( int ) ldap_create_page_control_value LDAP_P(( LDAP *ld, unsigned long pagesize, struct berval *cookie, struct berval *value )); LDAP_F( int ) ldap_create_page_control LDAP_P(( LDAP *ld, unsigned long pagesize, struct berval *cookie, int iscritical, LDAPControl **ctrlp )); LDAP_F( int ) ldap_parse_page_control LDAP_P(( LDAP *ld, LDAPControl **ctrls, unsigned long *count, struct berval **cookie )); LDAP_F( int ) ldap_parse_pageresponse_control LDAP_P(( LDAP *ld, LDAPControl *ctrl, unsigned long *count, struct berval *cookie )); ldap_create_page_control() uses ldap_create_page_control_value() and should be deprecated, IMHO, as it favors an inefficient style of using controls in C programming. ldap_parse_page_control() uses ldap_parse_pageresponse_control(); it is consistent with existing LDAP C API, but it should be deprecated as well for the same reason above. In general: ldap_create_*_control() may lead to poor programming, but they're not too bad by themselves; all in all there might be cases where the list of controls must be entirely malloc()'ed. ldap_create_*_control_value() should be used instead, whenever possible, with LDAPControl structs on the stack. Controls like manageDSAit, passwordpolicy and so don't even need this type of helper because they have empty value. ldap_parse_*_control() functions should also be deprecated; in fact, they imply control lookup in an array, and, in general, unnecessary allocation to return results. I consider unnecessary allocation, for example, when a berval is malloc()'ed to mark the distinction between no return value and an empty return value if the semantics of the control do not require that distinction. ldap_parse_*response_control() functions directly take a pointer to the LDAPControl structure in input; to further mark the distinction with older functions, we could just pass a pointer to the control value, as they do not need anything else: the OID is implicit in the function name, and the criticality __must__ be FALSE, so control response semantics cannot depend on the rest of the LDAPControl structure. Please check my commits and report. p. On Sun, 2006-01-08 at 22:35 +0100, Hans Leidekker wrote: > On Sunday 08 January 2006 19:16, Kurt D. Zeilenga wrote: > > > I would like to this be reworked using ppolicy API as a basis than > > Can you specify what you mean by "ppolicy API"? I presume you > mean taking a single control instead of an array?. And assert()ing > on faulty parameters maybe? Anything else? > > I see a function ldap_parse_passwordpolicy_control() in ppolicy.c > that does inline berval parsing, which contrasts with what Pierangelo > said about splitting it up. > > > the older sort/vlv control APIs (which should also be reworked). > > Parse routines shouldn't also find the control as this makes it > > harder to use in programs that include their own finding. > > Hmm, anything else? ;-) So if I take into account what Pierangelo > and you wrote I arrive at the following API: > > int > ldap_create_page_control( > LDAP *ld, > unsigned long pagesize, > struct berval *cookie, > int iscritical, > LDAPControl **ctrlp ); > > int > ldap_parse_page_control ( > LDAP *ld, > LDAPControl *ctrls, > unsigned long *count, > struct berval *cookie ); > > Changes relative to the previous version: > - change iscritical from a "const char" to an "int". > - accept an "LDAPControl *" instead of an "LDAPControl **" > - return a "struct berval *" instead of a "struct berval **". > > -Hans Ing. Pierangelo Masarati Responsabile Open Solution OpenLDAP Core Team SysNet s.n.c. Via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ------------------------------------------ Office: +39.02.23998309 Mobile: +39.333.4963172 Email: pierangelo.masarati@sys-net.it ------------------------------------------
On Sunday 08 January 2006 23:12, Pierangelo Masarati wrote: > I've committed something that looks like (ldap.h) Wow, you are fast. > Please check my commits and report. Looks good, thanks. I'll be testing this and I feel obliged now. I'll write you a man page for this if I find the time. -Hans
At 02:06 PM 1/8/2006, ando@sys-net.it wrote: >ldap_create_page_control() uses ldap_create_page_control_value() and >should be deprecated, IMHO, as it favors an inefficient style of using >controls in C programming. Why add routines that should be deprecated? Kurt
On Sun, 2006-01-08 at 23:49 +0000, Kurt@OpenLDAP.org wrote: > At 02:06 PM 1/8/2006, ando@sys-net.it wrote: > >ldap_create_page_control() uses ldap_create_page_control_value() and > >should be deprecated, IMHO, as it favors an inefficient style of using > >controls in C programming. > > Why add routines that should be deprecated? In general, I'm __preserving__ routines that should be deprecated, adding what I believe to be better replacements, and reworking the deprecated ones in terms of the non-deprecated. Paged results is an exception, because they are just being added and there appears to be already a commonly accepted API. Feel free to axe it instead of placing into LDAP_DEPRECATED. In any case, I feel those changes won't be released before 2.4, so chances are things will get reworked again. p. Ing. Pierangelo Masarati Responsabile Open Solution OpenLDAP Core Team SysNet s.n.c. Via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ------------------------------------------ Office: +39.02.23998309 Mobile: +39.333.4963172 Email: pierangelo.masarati@sys-net.it ------------------------------------------
At 11:00 PM 1/8/2006, Pierangelo Masarati wrote: >On Sun, 2006-01-08 at 23:49 +0000, Kurt@OpenLDAP.org wrote: >> At 02:06 PM 1/8/2006, ando@sys-net.it wrote: >> >ldap_create_page_control() uses ldap_create_page_control_value() and >> >should be deprecated, IMHO, as it favors an inefficient style of using >> >controls in C programming. >> >> Why add routines that should be deprecated? > >In general, I'm __preserving__ routines that should be deprecated, >adding what I believe to be better replacements, and reworking the >deprecated ones in terms of the non-deprecated. Paged results is an >exception, because they are just being added and there appears to be >already a commonly accepted API. Yes, but our deprecated API isn't the same as this "commonly accepted API". Our prototypes do differ from theirs. In particular, the sizes of some integer values passed by pointer may differ (depending on platform/compiler). There are also a other number of differences in the API, such as in related macros. >In any case, I feel those changes won't be >released before 2.4, Yes. >so chances are things will get reworked again. Possibly.
On Sunday 08 January 2006 23:12, Pierangelo Masarati wrote: In ldap_create_page_control_value() you do this: > if ( cookie == NULL ) { > cookie = &null_cookie; > } where cookie is passed by the caller, but he might depend on us not touching it, right? -Hans
> On Sunday 08 January 2006 23:12, Pierangelo Masarati wrote: > > In ldap_create_page_control_value() you do this: > >> if ( cookie == NULL ) { >> cookie = &null_cookie; >> } > > where cookie is passed by the caller, but he might depend > on us not touching it, right? In this case, I'm changing the value of the pointer, not its contents, so what I'm changing is passed by value and not by reference. I believe it works as intended. p. Ing. Pierangelo Masarati Responsabile Open Solution OpenLDAP Core Team SysNet s.n.c. Via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it ------------------------------------------ Office: +39.02.23998309 Mobile: +39.333.4963172 Email: pierangelo.masarati@sys-net.it ------------------------------------------
On Monday 09 January 2006 14:17, Pierangelo Masarati wrote: > > where cookie is passed by the caller, but he might depend > > on us not touching it, right? > > In this case, I'm changing the value of the pointer, not its contents, so > what I'm changing is passed by value and not by reference. I believe it > works as intended. Ah yes, I just opened my eyes and saw that. Sorry for the bother. -Hans
changed notes moved from Software Enhancements to Development
changed notes changed state Test to Closed
Hi OpenLdap-its, Can i get more info about this patch, such as: what is the openLDAP version can be used for this patch, where is the lastest url for this patch? thanks for the info YB Full_Name: Hans Leidekker Version: CVS OS: Linux URL: http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch Submission from: (NULL) (84.107.25.189) OpenLDAP does not provide any C API calls to encode/parse paged results requests/responses (RFC2696), although it supports the page control in the server. Augmenting the C API with such calls would fix this imbalance and also make porting certain client software to OpenLDAP easier. So here's a proposed a patch that implements the ldap_create_page_control and ldap_parse_page_control APIs: http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch
The ITS seems to answer your key questions. http://www.openldap.org/its/?findid=4314 It says the patch (with fixes) was applied to HEAD and was to released with 2.4. So the latest code should be in HEAD. It should also be in latest 2.4 release (2alpha). Kurt At 04:21 AM 8/16/2006, yoseph.basri@gmail.com wrote: >Hi OpenLdap-its, > >Can i get more info about this patch, such as: what is the openLDAP >version can be used for this patch, where is the lastest url for this >patch? > >thanks for the info > >YB > > >Full_Name: Hans Leidekker >Version: CVS >OS: Linux >URL: http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch >Submission from: (NULL) (84.107.25.189) > > > >OpenLDAP does not provide any C API calls to encode/parse paged results >requests/responses (RFC2696), although it supports the page control in >the server. Augmenting the C API with such calls would fix this imbalance >and also make porting certain client software to OpenLDAP easier. > >So here's a proposed a patch that implements the ldap_create_page_control >and ldap_parse_page_control APIs: > > http://mirzam.it.vu.nl/patches/hans-leidekker-060108.patch
moved from Development to Archive.Development
client API applied (with fixes) to HEAD for 2.4