Full_Name: Tianyin Xu Version: 2.4.33 OS: Ubuntu 12.04 (actually doesn't matter) URL: Submission from: (NULL) (2607:f720:1300:1241:590b:49c:889f:7b21) Hi, all, Still the verbosity idea. In the current version of slapd, when handling users' "concurrency" setting, slapd does not check the return value of "ldap_pvt_thread_set_concurrency" (essentially the return value of "pthread_setconcurrency") to see whether the setting is successful or not. So when the user setting is inappropriate (i.e., too big), slapd says nothing. But users have no way to be aware of such configuration failure. I suggest to check and notify users in this case. Here's the patch: --- ../openldap-2.4.33/servers/slapd/bconfig.c 2012-10-10 05:18:49.000000000 -0700 +++ servers/slapd/bconfig.c 2012-11-16 14:52:37.211421828 -0800 @@ -1530,7 +1530,14 @@ break; case CFG_CONCUR: - ldap_pvt_thread_set_concurrency(c->value_int); + + if(ldap_pvt_thread_set_concurrency(c->value_int) != 0) { + snprintf( c->cr_msg, sizeof( c->cr_msg ), + "concurrency=%d is not valid; using the default setting", + c->value_int ); + Debug(LDAP_DEBUG_ANY, "%s: %s.\n", c->log, c->cr_msg, 0 ); + + } break; case CFG_THREADS:
tixu@cs.ucsd.edu wrote: > Full_Name: Tianyin Xu > Version: 2.4.33 > OS: Ubuntu 12.04 (actually doesn't matter) > URL: > Submission from: (NULL) (2607:f720:1300:1241:590b:49c:889f:7b21) > > > Hi, all, > > Still the verbosity idea. In the current version of slapd, when handling users' > "concurrency" setting, slapd does not check the return value of > "ldap_pvt_thread_set_concurrency" (essentially the return value of > "pthread_setconcurrency") to see whether the setting is successful or not. > > So when the user setting is inappropriate (i.e., too big), slapd says nothing. > But users have no way to be aware of such configuration failure. I suggest to > check and notify users in this case. pthread_setconcurrency is a no-op on Linux systems. Even on systems where this setting might have an effect, it is generally unused. We have more important things to worry about than this. > Here's the patch: > > --- ../openldap-2.4.33/servers/slapd/bconfig.c 2012-10-10 > 05:18:49.000000000 -0700 > +++ servers/slapd/bconfig.c 2012-11-16 14:52:37.211421828 -0800 > @@ -1530,7 +1530,14 @@ > break; > > case CFG_CONCUR: > - ldap_pvt_thread_set_concurrency(c->value_int); > + > + if(ldap_pvt_thread_set_concurrency(c->value_int) != 0) > { > + snprintf( c->cr_msg, sizeof( c->cr_msg ), > + "concurrency=%d is not valid; using > the default setting", > + c->value_int ); > + Debug(LDAP_DEBUG_ANY, "%s: %s.\n", c->log, > c->cr_msg, 0 ); > + > + } > break; > > case CFG_THREADS: > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
On Sun, Nov 25, 2012 at 7:54 PM, Howard Chu <hyc@symas.com> wrote: > tixu@cs.ucsd.edu wrote: > >> Full_Name: Tianyin Xu >> Version: 2.4.33 >> OS: Ubuntu 12.04 (actually doesn't matter) >> URL: >> Submission from: (NULL) (2607:f720:1300:1241:590b:49c:**889f:7b21) >> >> >> Hi, all, >> >> Still the verbosity idea. In the current version of slapd, when handling >> users' >> "concurrency" setting, slapd does not check the return value of >> "ldap_pvt_thread_set_**concurrency" (essentially the return value of >> "pthread_setconcurrency") to see whether the setting is successful or not. >> >> So when the user setting is inappropriate (i.e., too big), slapd says >> nothing. >> But users have no way to be aware of such configuration failure. I >> suggest to >> check and notify users in this case. >> > > pthread_setconcurrency is a no-op on Linux systems. Even on systems where > this setting might have an effect, it is generally unused. We have more > important things to worry about than this. > > Yes, I understand that user configuration is never a high-priority issue. That's why I attached my patch which simply checks the return value, and prints out a log messge to notify users. The patch itself does not have any side effect to other part of code. I personally think the verbosity idea can improve the system reactions to users' configurations including this one and its #7443. It is safe (no side effect) and easy to do (simply adding some log messages), but can greatly improve the usability and friendlyness to configurations. If you agree, I'm happy to write patches and corresponding code. Thanks, Tianyin > Here's the patch: >> >> --- ../openldap-2.4.33/servers/**slapd/bconfig.c 2012-10-10 >> 05:18:49.000000000 -0700 >> +++ servers/slapd/bconfig.c 2012-11-16 14:52:37.211421828 -0800 >> @@ -1530,7 +1530,14 @@ >> break; >> >> case CFG_CONCUR: >> - ldap_pvt_thread_set_**concurrency(c->value_int); >> + >> + if(ldap_pvt_thread_set_**concurrency(c->value_int) >> != 0) >> { >> + snprintf( c->cr_msg, sizeof( c->cr_msg ), >> + "concurrency=%d is not valid; >> using >> the default setting", >> + c->value_int ); >> + Debug(LDAP_DEBUG_ANY, "%s: %s.\n", >> c->log, >> c->cr_msg, 0 ); >> + >> + } >> break; >> >> case CFG_THREADS: >> >> >> >> > > -- > -- Howard Chu > CTO, Symas Corp. http://www.symas.com > Director, Highland Sun http://highlandsun.com/hyc/ > Chief Architect, OpenLDAP http://www.openldap.org/**project/<http://www.openldap.org/project/> > -- Tianyin XU, http://cseweb.ucsd.edu/~tixu/
Tianyin Xu wrote: > > On Sun, Nov 25, 2012 at 7:54 PM, Howard Chu <hyc@symas.com > <mailto:hyc@symas.com>> wrote: > > tixu@cs.ucsd.edu <mailto:tixu@cs.ucsd.edu> wrote: > > Full_Name: Tianyin Xu > Version: 2.4.33 > OS: Ubuntu 12.04 (actually doesn't matter) > URL: > Submission from: (NULL) (2607:f720:1300:1241:590b:49c:__889f:7b21) > > > Hi, all, > > Still the verbosity idea. In the current version of slapd, when > handling users' > "concurrency" setting, slapd does not check the return value of > "ldap_pvt_thread_set___concurrency" (essentially the return value of > "pthread_setconcurrency") to see whether the setting is successful or not. > > So when the user setting is inappropriate (i.e., too big), slapd says > nothing. > But users have no way to be aware of such configuration failure. I > suggest to > check and notify users in this case. > > > pthread_setconcurrency is a no-op on Linux systems. Even on systems where > this setting might have an effect, it is generally unused. We have more > important things to worry about than this. > > > Yes, I understand that user configuration is never a high-priority issue. > That's why I attached my patch which simply checks the return value, and > prints out a log messge to notify users. The patch itself does not have any > side effect to other part of code. > > I personally think the verbosity idea can improve the system reactions to > users' configurations including this one and its #7443. It is safe (no side > effect) and easy to do (simply adding some log messages), but can greatly > improve the usability and friendlyness to configurations. If you agree, I'm > happy to write patches and corresponding code. We appreciate your desire to help, but as I said before, these two directives are essentially unused in 99.999% of deployments. If you're looking for code to improve, I suggest you review some of the other open reports in the ITS. > > Thanks, > Tianyin > > Here's the patch: > > --- ../openldap-2.4.33/servers/__slapd/bconfig.c 2012-10-10 > 05:18:49.000000000 -0700 > +++ servers/slapd/bconfig.c 2012-11-16 14:52:37.211421828 -0800 > @@ -1530,7 +1530,14 @@ > break; > > case CFG_CONCUR: > - ldap_pvt_thread_set___concurrency(c->value_int); > + > + > if(ldap_pvt_thread_set___concurrency(c->value_int) != 0) > { > + snprintf( c->cr_msg, sizeof( c->cr_msg ), > + "concurrency=%d is not > valid; using > the default setting", > + c->value_int ); > + Debug(LDAP_DEBUG_ANY, "%s: %s.\n", > c->log, > c->cr_msg, 0 ); > + > + } > break; > > case CFG_THREADS: > > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
moved from Incoming to Software Bugs