[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
Re: (ITS#7444) Check the return value of ldap_pvt_thread_set_concurrency
- To: openldap-its@OpenLDAP.org
- Subject: Re: (ITS#7444) Check the return value of ldap_pvt_thread_set_concurrency
- From: tixu@cs.ucsd.edu
- Date: Mon, 26 Nov 2012 09:27:51 GMT
- Auto-submitted: auto-generated (OpenLDAP-ITS)
--f46d04428c6e0fb88b04cf62891a
Content-Type: text/plain; charset=ISO-8859-1
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/
--f46d04428c6e0fb88b04cf62891a
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
<div class=3D"gmail_extra"><br><div class=3D"gmail_quote">On Sun, Nov 25, 2=
012 at 7:54 PM, Howard Chu <span dir=3D"ltr"><<a href=3D"mailto:hyc@syma=
s.com" target=3D"_blank">hyc@symas.com</a>></span> wrote:<br><blockquote=
class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc soli=
d;padding-left:1ex">
<div class=3D"im"><a href=3D"mailto:tixu@cs.ucsd.edu"; target=3D"_blank">tix=
u@cs.ucsd.edu</a> wrote:<br>
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex">
Full_Name: Tianyin Xu<br>
Version: 2.4.33<br>
OS: Ubuntu 12.04 (actually doesn't matter)<br>
URL:<br>
Submission from: (NULL) (2607:f720:1300:1241:590b:49c:<u></u>889f:7b21)<br>
<br>
<br>
Hi, all,<br>
<br>
Still the verbosity idea. In the current version of slapd, when handling us=
ers'<br>
"concurrency" setting, slapd does not check the return value of<b=
r>
"ldap_pvt_thread_set_<u></u>concurrency" (essentially the return =
value of<br>
"pthread_setconcurrency") to see whether the setting is successfu=
l or not.<br>
<br>
So when the user setting is inappropriate (i.e., too big), slapd says nothi=
ng.<br>
But users have no way to be aware of such configuration failure. I suggest =
to<br>
check and notify users in this case.<br>
</blockquote>
<br></div>
pthread_setconcurrency is a no-op on Linux systems. Even on systems where t=
his setting might have an effect, it is generally unused. We have more impo=
rtant things to worry about than this.<div class=3D"HOEnZb"><div><br></div>
</div></blockquote><div><br>Yes, I understand that user configuration is ne=
ver 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 p=
atch itself does not have any side effect to other part of code. <br>
<br>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 g=
reatly improve the usability and friendlyness to configurations. If you agr=
ee, I'm happy to write patches and corresponding code. <br>
<br>Thanks,<br>Tianyin<br><br>=A0</div><blockquote class=3D"gmail_quote" st=
yle=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div =
class=3D"HOEnZb"><div class=3D"h5">
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex">
Here's the patch:<br>
<br>
--- ../openldap-2.4.33/servers/<u></u>slapd/bconfig.c =A0 =A0 =A02012-10-10=
<br>
05:18:49.000000000 -0700<br>
+++ servers/slapd/bconfig.c =A0 =A0 2012-11-16 14:52:37.211421828 -0800<br>
@@ -1530,7 +1530,14 @@<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;<br>
<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0case CFG_CONCUR:<br>
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ldap_pvt_thread_set_<u></u>co=
ncurrency(c->value_int);<br>
+<br>
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if(ldap_pvt_thread_set_<u>=
</u>concurrency(c->value_int) !=3D 0)<br>
{<br>
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 snprintf( c-&=
gt;cr_msg, sizeof( c->cr_msg ),<br>
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 "concurrency=3D%d is not valid; using<br>
the default setting",<br>
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 c->value_int );<br>
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Debug(LDAP=
_DEBUG_ANY, "%s: %s.\n", c->log,<br>
c->cr_msg, 0 );<br>
+<br>
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;<br>
<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0case CFG_THREADS:<br>
<br>
<br>
<br>
</blockquote>
<br>
<br></div></div><span class=3D"HOEnZb"><font color=3D"#888888">
-- <br>
=A0 -- Howard Chu<br>
=A0 CTO, Symas Corp. =A0 =A0 =A0 =A0 =A0 <a href=3D"http://www.symas.com"; t=
arget=3D"_blank">http://www.symas.com</a><br>
=A0 Director, Highland Sun =A0 =A0 <a href=3D"http://highlandsun.com/hyc/"; =
target=3D"_blank">http://highlandsun.com/hyc/</a><br>
=A0 Chief Architect, OpenLDAP =A0<a href=3D"http://www.openldap.org/project=
/" target=3D"_blank">http://www.openldap.org/<u></u>project/</a><br>
</font></span></blockquote></div><br><br clear=3D"all"><br>-- <br><span sty=
le=3D"border-collapse:separate;color:rgb(0,0,0);font-family:'Times New =
Roman';font-style:normal;font-variant:normal;font-weight:normal;letter-=
spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;t=
ext-transform:none;white-space:normal;word-spacing:0px;font-size:medium"><s=
pan style=3D"color:rgb(102,102,102);font-family:arial;font-size:small">Tian=
yin XU,<br>
<a href=3D"http://cseweb.ucsd.edu/%7Etixu/"; target=3D"_blank">http://cseweb=
.ucsd.edu/~tixu/</a></span></span><br>
</div>
--f46d04428c6e0fb88b04cf62891a--