Full_Name: Eric Haszlakiewicz Version: cvsweb OS: RHEL5 URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (12.106.254.66) The ldap_search_ext, ldap_search_ext_s, ldap_search, ldap_search_s and ldap_search_st functions should use const for the attrs parameter. I checked revision 1.88 of libraries/libldap/search.c and it appears that those strings do not get modified so there is no reason that a list of string constants couldn't be passed in. I have a short patch that changes libraries/libldap/search.c and include/ldap.h (rev 1.349) to add the const qualifier.
hmm... I'm not quite sure how to attach a file to a ticket through the web interface, so I'm just going to paste the patches in here: --- ldap.h.1.349.orig 2010-04-02 15:32:34.113126200 -0500 +++ ldap.h 2010-04-02 15:33:55.252793400 -0500 @@ -1841,7 +1841,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **serverctrls, LDAPControl **clientctrls, @@ -1855,7 +1855,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **serverctrls, LDAPControl **clientctrls, @@ -1870,7 +1870,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly )); LDAP_F( int ) @@ -1879,7 +1879,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPMessage **res )); @@ -1889,7 +1889,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, struct timeval *timeout, LDAPMessage **res )); --- search.c.1.88.orig 2010-04-02 15:26:34.268550800 -0500 +++ search.c 2010-04-02 15:32:36.939198200 -0500 @@ -47,7 +47,7 @@ * attrsonly 1 => attributes only 0 => attributes and values * * Example: - * char *attrs[] = { "mail", "title", 0 }; + * const char *attrs[] = { "mail", "title", 0 }; * ldap_search_ext( ld, "dc=example,dc=com", LDAP_SCOPE_SUBTREE, "cn~=bob", * attrs, attrsonly, sctrls, ctrls, timeout, sizelimit, * &msgid ); @@ -58,7 +58,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -76,7 +76,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -139,7 +139,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -157,7 +157,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -210,14 +210,14 @@ * attrsonly 1 => attributes only 0 => attributes and values * * Example: - * char *attrs[] = { "mail", "title", 0 }; + * const char *attrs[] = { "mail", "title", 0 }; * msgid = ldap_search( ld, "dc=example,dc=com", LDAP_SCOPE_SUBTREE, "cn~=bob", * attrs, attrsonly ); */ int ldap_search( LDAP *ld, LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, int attrsonly ) + LDAP_CONST char **attrs, int attrsonly ) { BerElement *ber; ber_int_t id; @@ -246,7 +246,7 @@ LDAP_CONST char *base, ber_int_t scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, ber_int_t attrsonly, LDAPControl **sctrls, LDAPControl **cctrls, @@ -390,7 +390,7 @@ int ldap_search_st( LDAP *ld, LDAP_CONST char *base, int scope, - LDAP_CONST char *filter, char **attrs, + LDAP_CONST char *filter, LDAP_CONST char **attrs, int attrsonly, struct timeval *timeout, LDAPMessage **res ) { int msgid; @@ -419,7 +419,7 @@ LDAP_CONST char *base, int scope, LDAP_CONST char *filter, - char **attrs, + LDAP_CONST char **attrs, int attrsonly, LDAPMessage **res ) {
> hmm... I'm not quite sure how to attach a file to a ticket through the > web interface, In general, upload to ftp.openldap.org according to contribution instructions, and post a message with the URL; the way you provided it makes it pretty unusable. However, I think your patch will not be considered, although in principle perfectly acceptable, because an API change of this type would probably break a lot of code out there, without significant advantages. p.
-----Original Message----- >From: masarati@aero.polimi.it [mailto:masarati@aero.polimi.it] >In general, upload to ftp.openldap.org according to contribution >instructions, and post a message with the URL; the way you provided it >makes it pretty unusable. I saw the instructions about using the ftp site some time after I sent the patches. Sorry. The patch was just the obvious, mechanical change to add const to the attrs parameter in the functions that have it. Maybe a reminder to keep reading the rest of the page in the section on "patches" would be helpful. I got to the part that started talking about attribution of large changes, and assumed the rest of the page wasn't relevant to what I was trying to do. >However, I think your patch will not be considered, although in principle >perfectly acceptable, because an API change of this type would probably >break a lot of code out there, without significant advantages. Well, a couple of observations: 1) At least one other ldap implementation already does this. 2) I consider being able to turn on the compiler warnings that tell you about parameters that might be changed when do don't expect them to be a significant advantage. For many of the source trees I've worked on, doing so (and treating warnings as errors, to force them to be fixed) has helped uncover a variety of bugs. 3) If you're not being pedantic about warnings, and causing them to be treated as errors, this API change just results in an extra warning, which wouldn't actually break any code. 4) Adding "const" effectively and simply documents the fact that those parameters aren't going to be changed. If the API doesn't say so, it would be nice if that was at least explicitly mentions in the man page. At the moment, I had to go read an analyze the ldap code to figure that out. I'd rather spend my time working on my own code instead, and be able to simply use the great library you guys have created. :) Anyway, if the change doesn't get accepted it won't be the end of the world, but obviously I'm hoping you find the above convincing. eric
> Anyway, if the change doesn't get accepted it won't be the end of the > world, but obviously I'm hoping you find the above convincing. Just to clarify: if I were to design the API from scratch, I'd CONST those args... so I agree with your suggestion. p.
>From: masarati@aero.polimi.it [mailto:masarati@aero.polimi.it] > >> hmm... I'm not quite sure how to attach a file to a ticket through the >> web interface, > >In general, upload to ftp.openldap.org according to contribution >instructions, and post a message with the URL; the way you provided it >makes it pretty unusable. Fwiw, I uploaded the two patches as ehaszla-ldap.h-20100405.patch and ehaszla-search.c-20100405.patch eric
changed notes changed state Open to Suspended
changed notes
changed notes moved from Incoming to Software Enhancements
consolidated API change? not before re25