openldap-bugs July 2007

openldap-bugs@openldap.org

30 participants
126 discussions

Re: (ITS#4943) tpool.c pause vs. finish
by h.b.furuseth＠usit.uio.no 01 Jul '07

01 Jul '07

hyc(a)symas.com writes: >h.b.furuseth(a)usit.uio.no wrote: >> I wonder if we've been waiting for each other... Anyway, I've committed >> some pending fixes now that HEAD passes all tests for me. I've some >> more cleanup, but if we are close to a freeze I don't know if that means >> I should rush it in or wait until afterwards. > > Cleanup as in tidying the code, or as in fixing functionality? One cleanup (move thread start before queuing task in pool_submit(), one mini functionality: Make thread counts int instead of long, since pool_query() wants that. Oh, and make max_pending 0 or greater, since negative numbers confuse pool_query(). >> Another time when tls.c code loses information is if pointers are wider >> than long, which does happen. (Don't know how it is nowadays, but I >> remember plenty of postings about it some years ago.) > > Probably the OpenSSL API should have used a void * here. Too bad. Yup. I've seen mention it is coming in future version. > OK, suppose we use a pthread thread-specific key here. What are you > proposing for the unique value? An unsigned long counter, maintained by libldap_r. If the thread-local variable is zero (thus uninitialized), increment the counter and grab its value. >>>>> It's really >>>>> not hard. If we were to disallow increasing the number of threads at >>>>> runtime, we could simply allocate a block of thread stacks all at once, >>>>> which would then allow our thread IDs to be simple integers from 0-N. >> >> IIRC disallowing that would fix some other tpool problem too, but I >> don't remember what. > > I'm not really keen on that since I like having the ability to both increase > and decrease the setting dynamically. Me too, the only pro is it makes implementation simpler. >> Which led me to notice that the setstacksize call now _decreases_ the >> stack size on some hosts. I don't suppose that was intentional, so I >> guess it should be >> size_t cursize; >> if( pthread_attr_getstacksize( &attr, &cursize ) >> || cursize < LDAP_PVT_THREAD_STACK_SIZE ) >> pthread_attr_setstacksize( &attr, LDAP_PVT_THREAD_STACK_SIZE ); >> in thr_posix and I don't know what in other thr_* files. A RE24 change >> I guess, since reduces the max number of threads before slapd gets into >> trouble. > > I don't consider this a bug. If someone wants a larger thread stack > they can recompile to get it. Or perhaps we should make it a tunable > variable and add a config keyword for it. OK -- Regards, Hallvard

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by hyc＠symas.com 01 Jul '07

01 Jul '07

h.b.furuseth(a)usit.uio.no wrote: > I wonder if we've been waiting for each other... Anyway, I've committed > some pending fixes now that HEAD passes all tests for me. I've some > more cleanup, but if we are close to a freeze I don't know if that means > I should rush it in or wait until afterwards. Cleanup as in tidying the code, or as in fixing functionality? > >>> We did have code which compared thread IDs with '==' for >>> quite some time. Do you remember if anyone complained, or if the >>> switch to using ldap_pvt_thread_equal() was a "just in case" fix? >> Yes, I needed to use pthread_equal() on OS/390 since their thread type >> was a structure. > > I'm not sure, but I gather that's derived from the DCE threads reference > implementation where pthread_t has three members 'field1', 'field2' and > 'field3'. tls.c can't stuff all of them in a long, so the old one may > not get a unique ID. OTOH DCE has a pthread_getunique_np() call which > uses just the 2nd field as a unique integer thread ID, I expect we could > use that. Except I don't have a host to test such a change on. Dunno > why pthread_equal checks all three fields. I've asked on > comp.programming.threads. I don't remember which version of the spec OS/390 implemented, it was certainly old but it wasn't DCE. Might have been Draft 6 IIRC. > Also I note that the DCE struct will get four padding bytes on a 64-bit > host which aligns the struct at 8*n addresses, and the compiler is not > obliged to preserve these during struct copy. > > On the other hand DCE threads are based on an extremely outdated Posix > draft, so I don't know how relevant it is now. Yes, Draft 4, and I don't know of any systems that still use that exclusively. > Another time when tls.c code loses information is if pointers are wider > than long, which does happen. (Don't know how it is nowadays, but I > remember plenty of postings about it some years ago.) Probably the OpenSSL API should have used a void * here. Too bad. > [Using address of the thread's stack] >>>> That's what the LDAP_PVT_THREAD_STACK_SIZE macro tells you. > > As modified by PTHREAD_STACK_MIN and pthread_attr_getguardsize(). And > slight other variations, apparently. I tried on a few hosts and Solaris > would a few times use some extra space near the stack of a thread or two. > > OTOH we could allocate stack memory ourselves and then use > pthread_attr_setstack() to put the stack just where we want it. > > However this doesn't help tls.c with threads that have been created by > _other_ packages. And the SSL id_callback function apparently has no > way to return failure:-( > > All in all I still like a thread-specific key better - they are the > standard way of doing things like this. We only need it for pthreads > since the other thread types as far as I can tell have integer IDs. OK, suppose we use a pthread thread-specific key here. What are you proposing for the unique value? >>>> It's really >>>> not hard. If we were to disallow increasing the number of threads at >>>> runtime, we could simply allocate a block of thread stacks all at once, >>>> which would then allow our thread IDs to be simple integers from 0-N. > > IIRC disallowing that would fix some other tpool problem too, but I > don't remember what. I'm not really keen on that since I like having the ability to both increase and decrease the setting dynamically. > Which led me to notice that the setstacksize call now _decreases_ the > stack size on some hosts. I don't suppose that was intentional, so I > guess it should be > size_t cursize; > if( pthread_attr_getstacksize( &attr, &cursize ) > || cursize < LDAP_PVT_THREAD_STACK_SIZE ) > pthread_attr_setstacksize( &attr, LDAP_PVT_THREAD_STACK_SIZE ); > in thr_posix and I don't know what in other thr_* files. A RE24 change > I guess, since reduces the max number of threads before slapd gets into > trouble. I don't consider this a bug. If someone wants a larger thread stack they can recompile to get it. Or perhaps we should make it a tunable variable and add a config keyword for it. -- -- 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/

1 0

Re: Tranlucent Overlay (ITS#4889)
by ghenry＠suretecsystems.com 01 Jul '07

01 Jul '07

<quote who="Hallvard B Furuseth"> > ghenry(a)suretecsystems.com writes: >>> I have time, a manpage fix doesn't require testing:-) I'm just unsure >>> what it should say. Pierangelo's latest message kind of implied that >>> it >>> can be used with older overlays but not newer ones. >>> Should we say which ones? > > Well, yes. Shouldn't suddenly tell people they shouldn't be doing what > they are doing in cases when it's all right. But I don't know which > ones. Ping Pierangelo? > > -- > Hallvard >

1 0

Re: Tranlucent Overlay (ITS#4889)
by h.b.furuseth＠usit.uio.no 01 Jul '07

01 Jul '07

ghenry(a)suretecsystems.com writes: >> I have time, a manpage fix doesn't require testing:-) I'm just unsure >> what it should say. Pierangelo's latest message kind of implied that it >> can be used with older overlays but not newer ones. >> Should we say which ones? Well, yes. Shouldn't suddenly tell people they shouldn't be doing what they are doing in cases when it's all right. But I don't know which ones. -- Hallvard

1 0

Re: Tranlucent Overlay (ITS#4889)
by ghenry＠suretecsystems.com 01 Jul '07

01 Jul '07

<quote who="Hallvard B Furuseth"> > Gavin Henry writes: >> True. Also, when I raised this ticket I wasn't part of the team, so I >> can take on the man page fix if Hallvard doesn't have time. > > I have time, a manpage fix doesn't require testing:-) I'm just unsure > what it should say. Pierangelo's latest message kind of implied that it > can be used with older overlays but not newer ones. Should we say which ones? > > -- > Regards, > Hallvard >

1 0

Re: (ITS#4943) tpool.c pause vs. finish
by h.b.furuseth＠usit.uio.no 01 Jul '07

01 Jul '07

I wonder if we've been waiting for each other... Anyway, I've committed some pending fixes now that HEAD passes all tests for me. I've some more cleanup, but if we are close to a freeze I don't know if that means I should rush it in or wait until afterwards. >> We did have code which compared thread IDs with '==' for >> quite some time. Do you remember if anyone complained, or if the >> switch to using ldap_pvt_thread_equal() was a "just in case" fix? > > Yes, I needed to use pthread_equal() on OS/390 since their thread type > was a structure. I'm not sure, but I gather that's derived from the DCE threads reference implementation where pthread_t has three members 'field1', 'field2' and 'field3'. tls.c can't stuff all of them in a long, so the old one may not get a unique ID. OTOH DCE has a pthread_getunique_np() call which uses just the 2nd field as a unique integer thread ID, I expect we could use that. Except I don't have a host to test such a change on. Dunno why pthread_equal checks all three fields. I've asked on comp.programming.threads. Also I note that the DCE struct will get four padding bytes on a 64-bit host which aligns the struct at 8*n addresses, and the compiler is not obliged to preserve these during struct copy. On the other hand DCE threads are based on an extremely outdated Posix draft, so I don't know how relevant it is now. Another time when tls.c code loses information is if pointers are wider than long, which does happen. (Don't know how it is nowadays, but I remember plenty of postings about it some years ago.) [Using address of the thread's stack] >>> That's what the LDAP_PVT_THREAD_STACK_SIZE macro tells you. As modified by PTHREAD_STACK_MIN and pthread_attr_getguardsize(). And slight other variations, apparently. I tried on a few hosts and Solaris would a few times use some extra space near the stack of a thread or two. OTOH we could allocate stack memory ourselves and then use pthread_attr_setstack() to put the stack just where we want it. However this doesn't help tls.c with threads that have been created by _other_ packages. And the SSL id_callback function apparently has no way to return failure:-( All in all I still like a thread-specific key better - they are the standard way of doing things like this. We only need it for pthreads since the other thread types as far as I can tell have integer IDs. >>> It's really >>> not hard. If we were to disallow increasing the number of threads at >>> runtime, we could simply allocate a block of thread stacks all at once, >>> which would then allow our thread IDs to be simple integers from 0-N. IIRC disallowing that would fix some other tpool problem too, but I don't remember what. >> Hmm. I asked the wrong question, should have read some man pages first. >> Still, thr_cthreads.c & thr_lwp.c don't use LDAP_PVT_THREAD_STACK_SIZE. >> man pthread_attr_getstacksize says it sets the _minimum_ stack size, but >> perhaps several M is large enough that one can expect it won't get >> multiples of that. > > That kinda implies that no one has been using the cthreads or lwp support > recently, since I doubt their default stack size would work with back-bdb's > filterindex demands. Which led me to notice that the setstacksize call now _decreases_ the stack size on some hosts. I don't suppose that was intentional, so I guess it should be size_t cursize; if( pthread_attr_getstacksize( &attr, &cursize ) || cursize < LDAP_PVT_THREAD_STACK_SIZE ) pthread_attr_setstacksize( &attr, LDAP_PVT_THREAD_STACK_SIZE ); in thr_posix and I don't know what in other thr_* files. A RE24 change I guess, since reduces the max number of threads before slapd gets into trouble. -- Regards, Hallvard

1 0

Re: Tranlucent Overlay (ITS#4889)
by h.b.furuseth＠usit.uio.no 01 Jul '07

01 Jul '07

Gavin Henry writes: > True. Also, when I raised this ticket I wasn't part of the team, so I > can take on the man page fix if Hallvard doesn't have time. I have time, a manpage fix doesn't require testing:-) I'm just unsure what it should say. Pierangelo's latest message kind of implied that it can be used with older overlays but not newer ones. -- Regards, Hallvard

1 0

Re: Tranlucent Overlay (ITS#4889)
by ghenry＠suretecsystems.com 01 Jul '07

01 Jul '07

<quote who="Howard Chu"> > ghenry(a)suretecsystems.com wrote: > >> Depends on the time you have I suppose. >> >> I presume this isn't the case in 2.4? >> >> If so, maybe we should use your first sentence and refer them to 2.4? > > Referring people to 2.4 really isn't nice unless we have an actual release > to > refer them to. Which is another reason we need to get a 2.4 beta out ASAP. True. Also, when I raised this ticket I wasn't part of the team, so I can take on the man page fix if Hallvard doesn't have time. > Since it looks like no activity has occurred on closing out the remaining > devel > ITSs perhaps we should just freeze the current tree and roll out 2.4.5 > as-is. I think that would peak users interest again too. > -- > -- 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/ >

1 0

(ITS#5035) Warning patrol
by h.b.furuseth＠usit.uio.no 01 Jul '07

01 Jul '07

Full_Name: Hallvard B Furuseth Version: RE23, HEAD OS: URL: Submission from: (NULL) (129.240.202.105) Submitted by: hallvard #1. slapd/frontend.c:frontend_init() has the statement if ( slap_known_controls ) which is always true since slap_known_controls is an array. Maybe it should be if ( slap_known_controls[0] )? Except... controls.c says slap_known_controls[0] == NULL, which is not true - at least not at the time of the if-test above. What's that about? #2. The return value from op->o_callback->sc_cleanup() is never used. Should it be? I noticed because the pcache_op_cleanup() function does not return anything. What should it return? #3. RE23 struct sync_cookie.rid and struct syncinfo_s.si_rid are 'long', but are printed as 'int'. Could change the formats, but - any reason not to instead change their types to 'int' as in HEAD? I haven't looked at the HEAD syncrepl changes, so I don't know what other issues might be involved. #4. bconfig.c: Integer overflow in (1 << (sizeof(int)*8 - 1)) in slap_loglevel_get(). If anything else have been trusting signed integer overflow to resemble unsigned integers, it's time to cease that. The behaviour is undefined, and compilers make use of this fact to optimize. The loglevel functions are confused about types anyway. They collect masks in slap_mask_t, but the API uses an int for the masks. Either the API should use slap_mask_t or masks should be stored in (unsigned) ints. Not for 2.3, I think. #5. RE23 slurpd/st.c passes a time_t* to lutil_atol() which expects a long*. #6. ldbm_db_errcall() has wrong prototype for DB >= 4.3. Uses void(const DB_ENV *, const char *, char *) for DB_VERSION_X >= 0x040300, I guess it should copy back-bdb and (a) use const char* for last param and (b) test for DB_VERSION_FULL instead. Better late than never, I guess... #7. Other format bugs: - slapd/daemon.c: SLAP_EVENT_FD(i) is a ptrdiff_t printed as int. I take it SLAP_EPOLL_EV_PTRFD() should return a ber_socket_t like in its 'sl_sd' branch. - libldap/request.c: LDAP* given to %p should be cast to void*. #7. "Real" warnings drown in void* = function* warnings. Someday I want to go through the code and fill in (void*) casts. That doesn't make the code more standard, but can shut up compilers. I don't know if this should be imported to RE23 - maybe the chance of a new typo is greater than the chance of uncovering a bug:-)

1 0

Re: Tranlucent Overlay (ITS#4889)
by h.b.furuseth＠usit.uio.no 01 Jul '07

01 Jul '07

hyc(a)symas.com writes: > Referring people to 2.4 really isn't nice unless we have an actual > release to refer them to. Which is another reason we need to get a 2.4 > beta out ASAP. Since it looks like no activity has occurred on > closing out the remaining devel ITSs perhaps we should just freeze the > current tree and roll out 2.4.5 as-is. I'm working through some new and a few old issues now, as it happens. But I don't see any reason why that needs to hold up a beta. There's always time for another beta. -- Regards, Hallvard

1 0

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs July 2007