The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's no longer tests for gmtime_r() in configure. Is it time to cleanup a bit?
p.
masarati@aero.polimi.it writes:
The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's no longer tests for gmtime_r() in configure. Is it time to cleanup a bit?
Grepping some OpenLDAP versions, I'm not sure there ever was a test for it. #Define it if everything protected by gmtime_mutex can be avoided. (gmtime_r, localtime_r, anything else?) After checking that the #ifdef HAVE_GMTIME_R branches (still) work, of course. Should probably also check that the prototype is as expected, in case there have been multiple versions as with 'int strerror_r()' vs 'char *strerror_r()'.
masarati@aero.polimi.it writes:
The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's no longer tests for gmtime_r() in configure. Is it time to cleanup a bit?
Grepping some OpenLDAP versions, I'm not sure there ever was a test for it. #Define it if everything protected by gmtime_mutex can be avoided. (gmtime_r, localtime_r, anything else?) After checking that the #ifdef HAVE_GMTIME_R branches (still) work, of course. Should probably also check that the prototype is as expected, in case there have been multiple versions as with 'int strerror_r()' vs 'char *strerror_r()'.
Builds fine with -DHAVE_GMTIME_R=1; on my Linux gmtime_r() is defined according to its use in OpenLDAP, but I have no means to check for conformance in other systems.
p.
masarati@aero.polimi.it schrieb:
masarati@aero.polimi.it writes:
The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's no longer tests for gmtime_r() in configure. Is it time to cleanup a bit?
Grepping some OpenLDAP versions, I'm not sure there ever was a test for it. #Define it if everything protected by gmtime_mutex can be avoided. (gmtime_r, localtime_r, anything else?) After checking that the #ifdef HAVE_GMTIME_R branches (still) work, of course. Should probably also check that the prototype is as expected, in case there have been multiple versions as with 'int strerror_r()' vs 'char *strerror_r()'.
Builds fine with -DHAVE_GMTIME_R=1; on my Linux gmtime_r() is defined according to its use in OpenLDAP, but I have no means to check for conformance in other systems.
possibly gmtime_r() as well as localtime_r() and other *_r() library calls could be handled similar to ctime_r() in pkg/ldap/libraries/libldap/util-int.c to assure compatibility regarding systems that do support the *_r version...
I've already tested this (just for fun) using localtime_r(): therefor I've successfully defined an additional autoconf macro in build/openldap.m4. Although autoreconf followed by ./config.status seemed to work fine I'm not sure whether I've understood the OL build system correctly.
Cheers Daniel
On Aug 18, 2009, at 5:20 AM, masarati@aero.polimi.it wrote:
The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's no longer tests for gmtime_r() in configure. Is it time to cleanup a bit?
p.
I note that OpenLDAP Software uses a single configure script to generate configuration information used for building both thread-aware and thread-unaware programs and libraries. Hence, special care must be taken the generated configuration information is valid for both purposes. In some cases, it's easier just to avoid the differences (such as by using (or not) certain system interfaces).
It likely would be better to have two (thread-aware/thread-unaware) or three (common/thread-aware/thread-unaware) configure scripts.
-- Kurt
On Aug 18, 2009, at 5:20 AM, masarati@aero.polimi.it wrote:
The code is littered with tests like #ifdef HAVE_GMTIME_R, but there's no longer tests for gmtime_r() in configure. Is it time to cleanup a bit?
p.
I note that OpenLDAP Software uses a single configure script to generate configuration information used for building both thread-aware and thread-unaware programs and libraries. Hence, special care must be taken the generated configuration information is valid for both purposes. In some cases, it's easier just to avoid the differences (such as by using (or not) certain system interfaces).
It likely would be better to have two (thread-aware/thread-unaware) or three (common/thread-aware/thread-unaware) configure scripts.
I'm inclined towards defining a private function, with an interface analogous to that of the tread-safe function, and use it all times, re-entrant as appropriate. If the re-entrant function is available, a #define could be used instead. Otherwise, the wrapper would take care of the rest. This would solve the issue once for all, and clean up quite a bit the code, as any mutex would no longer need to be exposed.
p.
masarati@aero.polimi.it writes:
I'm inclined towards defining a private function, with an interface analogous to that of the tread-safe function, and use it all times, re-entrant as appropriate. If the re-entrant function is available, a #define could be used instead. Otherwise, the wrapper would take care of the rest. This would solve the issue once for all, and clean up quite a bit the code, as any mutex would no longer need to be exposed.
That would mean moving lutil_gettime() (and its caller lutil_csnstr()?) to libldap, so libldap_r can use the mutex.
But not exposing gmtime_mutex would break third-partly modules that use gmtime()/localtime() correctly, which means protecting them with gmtime_mutex. We could however register gmtime_mutex in libldap, similar to registering which memory functions to use.
That would mean moving lutil_gettime() (and its caller lutil_csnstr()?) to libldap, so libldap_r can use the mutex.
Well, I was about to discuss this. In fact, there's a few comments in the code that seem to indicate the gmtime_mutex is used for other purposes than calling gmtime(3) safely. I mean: I plan to use gmtime(3) this way,
#ifdef USE_GMTIME_R #define ldap_pvt_gmtime(t,tm) gmtime_r((t), (tm)) #else struct tm *ldap_pvt_gmtime(time_t *t, struct tm *tm) { struct tm *tm_ptr; #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_lock( &gmtime_mutex ); #endif tm_ptr = gmtime( t ); *tm = *tm_ptr; #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_unlock( &gmtime_mutex ); #endif return tm; }
This means that the result of calling ldap_pvt_gmtime() will always be safe. However, somewhere in the code there was things like
ldap_pvt_thread_mutex_lock( &gmtime_mutex ); // do something that implies calling gmtime() ldap_pvt_thread_mutex_unlock( &gmtime_mutex );
I guess the only reason was to avoid copying the contents of the structure returned by gmtime(3). I need to check whether my assumption is correct or not. Significant cases include calling lutil_csnstr().
But not exposing gmtime_mutex would break third-partly modules that use gmtime()/localtime() correctly, which means protecting them with gmtime_mutex. We could however register gmtime_mutex in libldap, similar to registering which memory functions to use.
Right. But as far as I can tell, this is already the case of other mutex'es, including ldap_int_ctime_mutex, which is static, and other ldap_int_*_mutex'es, that are internal to libldap.
I'd rather favor exposing an API that allows to lock/unlock (trylock?) hidden mutexes. Although I fear this could open a can of worms.
p.
struct tm *ldap_pvt_gmtime(
const /* and LDAP_CONST in the .h file */
time_t *t, struct tm *tm){ struct tm *tm_ptr; #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_lock( &gmtime_mutex ); #endif tm_ptr = gmtime( t );
if (tm_ptr == NULL) tm = NULL; else
*tm = *tm_ptr;#ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_unlock( &gmtime_mutex ); #endif return tm; }
But not exposing gmtime_mutex would break third-partly modules that use gmtime()/localtime() correctly, which means protecting them with gmtime_mutex. We could however register gmtime_mutex in libldap, similar to registering which memory functions to use.
Right. But as far as I can tell, this is already the case of other mutex'es, including ldap_int_ctime_mutex, which is static, and other ldap_int_*_mutex'es, that are internal to libldap.
Still shouldn't break existing code which does work. Not in a minor release at least.
I'd rather favor exposing an API that allows to lock/unlock (trylock?) hidden mutexes. Although I fear this could open a can of worms.
Yes, it's a problem when several packages all want to "own" the mutex protecting a function (or rather, protecting its static data).
The friendliest way to handle that is an API which (a) accepts that the caller passes it the mutexes to set up (and maybe the mutex-lock functions too), and (b) provides useful defaults so callers don't need to do that.
I'm seeing a lot of warnings about lutil_tm declaration inside function params when compiling a fresh HEAD now. Seems to me there's something missing from these recent commits.
masarati@aero.polimi.it wrote:
That would mean moving lutil_gettime() (and its caller lutil_csnstr()?) to libldap, so libldap_r can use the mutex.
Well, I was about to discuss this. In fact, there's a few comments in the code that seem to indicate the gmtime_mutex is used for other purposes than calling gmtime(3) safely. I mean: I plan to use gmtime(3) this way,
#ifdef USE_GMTIME_R #define ldap_pvt_gmtime(t,tm) gmtime_r((t), (tm)) #else struct tm *ldap_pvt_gmtime(time_t *t, struct tm *tm) { struct tm *tm_ptr; #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_lock(&gmtime_mutex ); #endif tm_ptr = gmtime( t ); *tm = *tm_ptr; #ifdef LDAP_R_COMPILE ldap_pvt_thread_mutex_unlock(&gmtime_mutex ); #endif return tm; }
This means that the result of calling ldap_pvt_gmtime() will always be safe. However, somewhere in the code there was things like
ldap_pvt_thread_mutex_lock(&gmtime_mutex ); // do something that implies calling gmtime() ldap_pvt_thread_mutex_unlock(&gmtime_mutex );I guess the only reason was to avoid copying the contents of the structure returned by gmtime(3). I need to check whether my assumption is correct or not. Significant cases include calling lutil_csnstr().
I'm seeing a lot of warnings about lutil_tm declaration inside function params when compiling a fresh HEAD now. Seems to me there's something missing from these recent commits.
I need to carefully review where the new functions are used instead of the old ones. This is because the new functions are in libldap, while the old ones were in liblutil. I had to move them in libldap so that the mutex could be initialized appropriately. I only moved those functions that needed to be there. Other time-related, and significantly those using the lutil_tm struct, remained in liblutil. Probably ldap_pvt.h needs to include lutil.h. Suggestions?
p.
masarati@aero.polimi.it writes:
Other time-related, and significantly those using the lutil_tm struct, remained in liblutil. Probably ldap_pvt.h needs to include lutil.h. Suggestions?
I fixed it. The gcc warning explained clearly enough what was wrong.
I still disagree that moving gmtime_mutex out of slapd was the right thing to do in the middle of RE24 though. Though i suppose we could '#define gmtime_mutex ldap_int_gmtime_mutex' so slapd at least remains source-code compatible.
masarati@aero.polimi.it writes:
Other time-related, and significantly those using the lutil_tm struct, remained in liblutil. Probably ldap_pvt.h needs to include lutil.h. Suggestions?
I fixed it.
Thanks
The gcc warning explained clearly enough what was wrong.
I meant: in terms of opportunity. You can fix it directly by declaring the structure, or by anticipating the inclusion of lutil.h, since ldap_pvt.h depends on it.
I still disagree that moving gmtime_mutex out of slapd was the right thing to do in the middle of RE24 though. Though i suppose we could '#define gmtime_mutex ldap_int_gmtime_mutex' so slapd at least remains source-code compatible.
Mmmmh, ldap_int_gmtime_mutex is (and should in the long term remain) static. I see your point in hiding a symbol within bugfix releases, but I wonder how many applications cared about that in the first place, and how many developers are aware of the issue.
In general, I realize the library design needs to take into account two different issues related to re-entrancy:
1) the need for thread-safe behavior of libldap when usen in multithread clients (libldap_r)
2) the need to cure potential flaws in non re-entrant libraries OpenLDAP's depend on.
The two issues are orthogonal, in the sense that we can carefully design libldap's thread-safety, and provide indications about how to design thread-safe applications that use it.
As an entirely separate issue, we need to provide means to make thread-safe the use of potentially unsafe functions that we need. This is a bit harder, because:
a) we may need to use potentially unsafe functions in pieces of code not necessarily designed for thread-safety (e.g. liblutil in this case)
b) we may need to cooperate with other protections put in place by applications.
In any case, protection needs mutexes, and if we own them, we need to initialize them.
Usually, we put these things in libldap because it initializes itself (perhaps in a clumsy manner, by requiring to call at least a routine before threads are spawn), but this is good for case (1), libldap's thread-safe behavior. For case (2), curing re-entrance issues, I'm not sure what we'd do.
p.
masarati@aero.polimi.it writes:
The gcc warning explained clearly enough what was wrong.
I meant: in terms of opportunity. You can fix it directly by declaring the structure, or by anticipating the inclusion of lutil.h, since ldap_pvt.h depends on it.
Fine by me either way. Users of the new function depend on lutil.h, the rest of ldap_pvt.h does not.
I still disagree that moving gmtime_mutex out of slapd was the right thing to do in the middle of RE24 though. Though i suppose we could '#define gmtime_mutex ldap_int_gmtime_mutex' so slapd at least remains source-code compatible.
Mmmmh, ldap_int_gmtime_mutex is (and should in the long term remain) static.
Um, this is blowing up to a bit bigger issue than warranted, but anyway:
Long term, possibly - but only if we provide it to other packages somehow or accept another mutex from them. See below. But in RE24, it _was_ exported.
I see your point in hiding a symbol within bugfix releases, but I wonder how many applications cared about that in the first place, and how many developers are aware of the issue.
Either we try to be upgrade-friendly or not. Probably very few third-party modules are affected by this particular issue, just like very few are affected by many other particular issues. But these improbabilities do add up until we get an upgrade-unfriendly package.
That doesn't translate to an absolute freeze of e.g. slap.h between major releases, but we can at least avoid changes that could just as well be done in a compatible way. Previously there was one correct way to do it, now there is one different correct way, and - quite unnecessarily - there is no overlap beween the two.
As for this particular mutex and the corresponding ctime mutex, all threaded packages using gmtime and localtime must cooperate in order to get correct behavior. For example:
nm /usr/lib/lib<whatever>.a | egrep 'localtime|gmtime|ctime' | sort -u
/usr/lib/libsasl2.a: ctime localtime /usr/lib/libssl.a: gmtime localtime /usr/lib/libcrypto.a OPENSSL_gmtime X509_gmtime_adj gmtime_r localtime
I haven't looked too closely at the code, but possibly there is no way for a program using OpenSSL or Cyrus SASL to use the non-_r functions correctly. Didn't find any mutex protecting them, anyway.
Come to think of it, this might explain some of the syncrepl timing problems.
Anyway, if they all use the time functions, they must all use the same mutex or the same wrapper function.
In general, I realize the library design needs to take into account two different issues related to re-entrancy:
- the need for thread-safe behavior of libldap when usen in multithread
clients (libldap_r)
Yup. Which we really need to get around to officially supporting. If nothing else, because it gets silly to complain about other non-reentrant third-party code.
- the need to cure potential flaws in non re-entrant libraries OpenLDAP's
depend on.
The general cure for (2) is to run slapd single-threaded. Specific such issues of re-entrancy will have to be handled on a case-by-case basis.
(...) As an entirely separate issue, we need to provide means to make thread-safe the use of potentially unsafe functions that we need. This is a bit harder, because:
a) we may need to use potentially unsafe functions in pieces of code not necessarily designed for thread-safety (e.g. liblutil in this case)
b) we may need to cooperate with other protections put in place by applications.
Yes. And we need to _not_ insist that other packages do things Our Way. 3 packages that all insist that others do it their way cannot cooperate.
masarati@aero.polimi.it writes:
I still disagree that moving gmtime_mutex out of slapd was the right thing to do in the middle of RE24 though. Though i suppose we could '#define gmtime_mutex ldap_int_gmtime_mutex' so slapd at least remains source-code compatible.
Mmmmh, ldap_int_gmtime_mutex is (and should in the long term remain) static.
Um, this is blowing up to a bit bigger issue than warranted, but anyway:
Long term, possibly - but only if we provide it to other packages somehow or accept another mutex from them. See below. But in RE24, it _was_ exported.
Quick fix: - call it ldap_pvt_gmtime_mutex (non static); - leave it in libldap; - #define gmtime_mutex ldap_pvt_gmtime_mutex in slap.h Things can be rearranged differently in the next release.
I see your point in hiding a symbol within bugfix releases, but I wonder how many applications cared about that in the first place, and how many developers are aware of the issue.
Either we try to be upgrade-friendly or not. Probably very few third-party modules are affected by this particular issue, just like very few are affected by many other particular issues. But these improbabilities do add up until we get an upgrade-unfriendly package.
That doesn't translate to an absolute freeze of e.g. slap.h between major releases, but we can at least avoid changes that could just as well be done in a compatible way. Previously there was one correct way to do it, now there is one different correct way, and - quite unnecessarily - there is no overlap beween the two.
As for this particular mutex and the corresponding ctime mutex, all threaded packages using gmtime and localtime must cooperate in order to get correct behavior. For example:
nm /usr/lib/lib<whatever>.a | egrep 'localtime|gmtime|ctime' | sort -u
/usr/lib/libsasl2.a: ctime localtime /usr/lib/libssl.a: gmtime localtime /usr/lib/libcrypto.a OPENSSL_gmtime X509_gmtime_adj gmtime_r localtime
I haven't looked too closely at the code, but possibly there is no way for a program using OpenSSL or Cyrus SASL to use the non-_r functions correctly. Didn't find any mutex protecting them, anyway.
Come to think of it, this might explain some of the syncrepl timing problems.
Anyway, if they all use the time functions, they must all use the same mutex or the same wrapper function.
In general, I realize the library design needs to take into account two different issues related to re-entrancy:
- the need for thread-safe behavior of libldap when usen in multithread
clients (libldap_r)
Yup. Which we really need to get around to officially supporting. If nothing else, because it gets silly to complain about other non-reentrant third-party code.
- the need to cure potential flaws in non re-entrant libraries
OpenLDAP's depend on.
The general cure for (2) is to run slapd single-threaded. Specific such issues of re-entrancy will have to be handled on a case-by-case basis.
(...) As an entirely separate issue, we need to provide means to make thread-safe the use of potentially unsafe functions that we need. This is a bit harder, because:
a) we may need to use potentially unsafe functions in pieces of code not necessarily designed for thread-safety (e.g. liblutil in this case)
b) we may need to cooperate with other protections put in place by applications.
Yes. And we need to _not_ insist that other packages do things Our Way. 3 packages that all insist that others do it their way cannot cooperate.
Well, I'm open to accepting 3rd party ways to protect things. We usually try to use 3rd party code the way it is intended, as far as I understand. Of course, if 3rd party code overlooked issues, we need to do something else.
p.
-
Daniel -
Hallvard B Furuseth -
Howard Chu -
Kurt Zeilenga -
masarati＠aero.polimi.it