Re: (ITS#7444) Check the return value of ldap_pvt_thread_set_concurrency

[hyc@symas.com]

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/