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.