10:41 a.m.
hyc(a)OpenLDAP.org wrote:
Update of /repo/OpenLDAP/pkg/ldap/libraries/libldap
Modified Files:
result.c 1.149 -> 1.150
Log Message:
Fairly sure this is what the TIMEOUT option should always have been for
Not strictly related, but does it make sense to have a malloc for a
struct timeval in global/per handler data? Wouldn't it be any better to
use tv_sec = -1 to indicate no timeout? We should be able to change
that, since the struct is opaque...
p.
Ing. Pierangelo Masarati
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(a)sys-net.it
------------------------------------------
11:04 a.m.
New subject: commit: ldap/libraries/libldap result.c
Pierangelo Masarati wrote:
hyc(a)OpenLDAP.org wrote:
> Update of /repo/OpenLDAP/pkg/ldap/libraries/libldap
>
> Modified Files:
> result.c 1.149 -> 1.150
>
> Log Message:
> Fairly sure this is what the TIMEOUT option should always have been for
Not strictly related, but does it make sense to have a malloc for a
struct timeval in global/per handler data? Wouldn't it be any better to
use tv_sec = -1 to indicate no timeout? We should be able to change
that, since the struct is opaque...
Good idea.
--
-- Howard Chu
Chief Architect, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc
Chief Architect, OpenLDAP http://www.openldap.org/project/
11:15 a.m.
New subject: commit: ldap/libraries/libldap result.c
Pierangelo Masarati wrote:
hyc(a)OpenLDAP.org wrote:
> Update of /repo/OpenLDAP/pkg/ldap/libraries/libldap
>
> Modified Files:
> result.c 1.149 -> 1.150
>
> Log Message:
> Fairly sure this is what the TIMEOUT option should always have been for
Not strictly related, but does it make sense to have a malloc for a
struct timeval in global/per handler data? Wouldn't it be any better to
use tv_sec = -1 to indicate no timeout? We should be able to change
that, since the struct is opaque...
Hm. What about changing the get_option call? Currently it mallocs a timeval
and returns a pointer to that. It ought to just copy the contents of the
timeval. They've only been documented in HEAD; can we change it or would that
break something?
--
-- Howard Chu
Chief Architect, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc
Chief Architect, OpenLDAP http://www.openldap.org/project/
4:11 a.m.
New subject: commit: ldap/libraries/libldap result.c
Howard Chu writes:
Pierangelo Masarati wrote:
> hyc(a)OpenLDAP.org wrote:
>> Update of /repo/OpenLDAP/pkg/ldap/libraries/libldap
>>
>> Modified Files:
>> result.c 1.149 -> 1.150
>>
>> Log Message:
>> Fairly sure this is what the TIMEOUT option should always have been for
>
> Not strictly related, but does it make sense to have a malloc for a
> struct timeval in global/per handler data? Wouldn't it be any better to
> use tv_sec = -1 to indicate no timeout? We should be able to change
> that, since the struct is opaque...
Hm. What about changing the get_option call? Currently it mallocs a timeval
and returns a pointer to that. It ought to just copy the contents of the
timeval. They've only been documented in HEAD; can we change it or would that
break something?
Do you mean ldap_get_option()? It generally returns malloced data,
IIRC. "undocumented" just means the code has been the documentation, so
I presume a change will break any old code which uses it.
--
Regards,
Hallvard
4:34 a.m.
New subject: commit: ldap/libraries/libldap result.c
Hallvard B Furuseth wrote:
Howard Chu writes:
> Pierangelo Masarati wrote:
>> Not strictly related, but does it make sense to have a malloc for a
>> struct timeval in global/per handler data? Wouldn't it be any better to
>> use tv_sec = -1 to indicate no timeout? We should be able to change
>> that, since the struct is opaque...
> Hm. What about changing the get_option call? Currently it mallocs
a timeval
> and returns a pointer to that. It ought to just copy the contents of the
> timeval. They've only been documented in HEAD; can we change it or would that
> break something?
Do you mean ldap_get_option()? It generally returns malloced data,
IIRC. "undocumented" just means the code has been the documentation, so
I presume a change will break any old code which uses it.
Yes to all of the above. But since LDAP_OPT_TIMEOUT was completely
unimplemented before, I don't think it really makes any difference. I.e., the
option was not in the LDAPv2 API or the LDAPv3 draft API, and it never
actually did anything. Why would it have ever gotten used?
--
-- Howard Chu
Chief Architect, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc
Chief Architect, OpenLDAP http://www.openldap.org/project/
8:27 a.m.
New subject: commit: ldap/libraries/libldap result.c
Howard Chu writes:
Hallvard B Furuseth wrote:
> Do you mean ldap_get_option()? It generally returns malloced data,
> IIRC. "undocumented" just means the code has been the documentation,
> so I presume a change will break any old code which uses it.
Yes to all of the above. But since LDAP_OPT_TIMEOUT was completely
unimplemented before, I don't think it really makes any
difference. I.e., the option was not in the LDAPv2 API or the LDAPv3
draft API, and it never actually did anything. Why would it have ever
gotten used?
Mm. LDAP_OPT_NETWORK_TIMEOUT was implemented, though. So the change
breaks code which uses that option. And ldap_get_option() of the two
options should work the same way.
I too would have preferred if ldap_get_option() did less mallocing
(since there would be less cleanup required afterwards), but I think the
right place to fix it would be in the Next Generation API we sometimes
mumble about.
--
Regards,
Hallvard
11:49 a.m.
New subject: commit: ldap/libraries/libldap result.c
Hallvard B Furuseth wrote:
Howard Chu writes:
> Hallvard B Furuseth wrote:
>> Do you mean ldap_get_option()? It generally returns malloced data,
not for integers or so; only for strings and structures, which may be
fine for strings but sounds a waste for fixed size structures.
>> IIRC. "undocumented" just means the code has been
the documentation,
>> so I presume a change will break any old code which uses it.
> Yes to all of the above. But since LDAP_OPT_TIMEOUT was completely
> unimplemented before, I don't think it really makes any
> difference. I.e., the option was not in the LDAPv2 API or the LDAPv3
> draft API, and it never actually did anything. Why would it have ever
> gotten used?
Mm. LDAP_OPT_NETWORK_TIMEOUT was implemented, though. So the change
breaks code which uses that option. And ldap_get_option() of the two
options should work the same way.
I too would have preferred if ldap_get_option() did less mallocing
(since there would be less cleanup required afterwards), but I think the
right place to fix it would be in the Next Generation API we sometimes
mumble about.
I think the best solution would be to change the way timeouts are
stored, leave the old API in place (i.e. malloc stuff returned by
ldap_get_option() for LDAP_OPT_*TIMEOUT, and add new LDAP_OPT_X_*TIMEOUT
(call it whatever makes sense) that don't return malloc'ed stuff.
p.
Ing. Pierangelo Masarati
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(a)sys-net.it
------------------------------------------
2:05 p.m.
New subject: commit: ldap/libraries/libldap result.c
Pierangelo Masarati wrote:
Hallvard B Furuseth wrote:
> Mm. LDAP_OPT_NETWORK_TIMEOUT was implemented, though. So the change
> breaks code which uses that option. And ldap_get_option() of the two
> options should work the same way.
>
> I too would have preferred if ldap_get_option() did less mallocing
> (since there would be less cleanup required afterwards), but I think the
> right place to fix it would be in the Next Generation API we sometimes
> mumble about.
I think the best solution would be to change the way timeouts are
stored, leave the old API in place (i.e. malloc stuff returned by
ldap_get_option() for LDAP_OPT_*TIMEOUT, and add new LDAP_OPT_X_*TIMEOUT
(call it whatever makes sense) that don't return malloc'ed stuff.
I've restored the malloc behavior for LDAP_OPT_*TIMEOUT. I wasn't too keen on
adding new Option flags for retrieving the same setting, so I left that alone.
--
-- Howard Chu
Chief Architect, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc
Chief Architect, OpenLDAP http://www.openldap.org/project/
5839
days inactive
5840
days old
7 comments
3 participants
participants (3)
-
Hallvard B Furuseth -
Howard Chu -
Pierangelo Masarati