[From nobody Tue Nov 26 14:30:24 2013 From: Michal Minar <reviewboard@gallagherhome.com> Precedence: list MIME-Version: 1.0 To: "Michal Minar" <miminar@redhat.com>, "Jan Safranek" <jsafrane@redhat.com>, "OpenLMI Developers" <openlmi-reviews@lists.fedorahosted.org> References: <20131126111549.234376.884@ex-std-node147.prod.rhcloud.com> In-Reply-To: <20131126111549.234376.884@ex-std-node147.prod.rhcloud.com> Date: Tue, 26 Nov 2013 11:31:08 -0000 Reply-To: Michal Minar <miminar@redhat.com>, openlmi-reviews@lists.fedorahosted.org Message-ID: <20131126113108.234376.88241@ex-std-node147.prod.rhcloud.com> Content-Type: multipart/alternative; boundary="===============3066450856508988246==" Subject: Re: Review Request 1291: indicationmgr: fixed deadlock when re-starting the namager thread. Message: 63 --===============3066450856508988246== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1291/#review1793 ----------------------------------------------------------- Ship it! src/indmanager/ind_manager.c <http://reviewboard-openlmi.rhcloud.com/r/1291/#comment1030> I'd just mention, that the mutex must not be locked by the thread we want to cancel. Otherwise both sentences suggest that we want to have the mutex locked forever which does not make sense. - Michal Minar On Nov. 26, 2013, 11:15 a.m., Jan Safranek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard-openlmi.rhcloud.com/r/1291/ > ----------------------------------------------------------- > > (Updated Nov. 26, 2013, 11:15 a.m.) > > > Review request for OpenLMI Developers. > > > Repository: openlmi-providers > > > Description > ------- > > indicationmgr: fixed deadlock when re-starting the namager thread. > > When the indication manager thread was stopped, it destroys its mutex and > creates new one. If pthread_mutex_destroy() fails because the mutex is > locked, the new mutex is not created -> the old one is used, but it's already > locked -> deadlock. > > So let's make sure the mutex is unlocked when destroying the thread. > > Rewrite of indication manager thread is needed to have the manage() function > interruptible when waiting for events, e.g. using a pipe and select(), > this pthread_cancel leads to memory leaks. > > > Diffs > ----- > > src/indmanager/ind_manager.c 7d6bc7545647589178ed2a858c0d136c4806b7b0 > > Diff: http://reviewboard-openlmi.rhcloud.com/r/1291/diff/ > > > Testing > ------- > > > Thanks, > > Jan Safranek > > --===============3066450856508988246== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit <html> <body> <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;"> <table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;"> <tr> <td> This is an automatically generated e-mail. To reply, visit: <a href="http://reviewboard-openlmi.rhcloud.com/r/1291/">http://reviewboard-openlmi.rhcloud.com/r/1291/</a> </td> </tr> </table> <br /> <p>Ship it!</p> <div> <table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;"> <thead> <tr> <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;"> <a href="http://reviewboard-openlmi.rhcloud.com/r/1291/diff/1/?file=6684#file6684line1221" style="color: black; font-weight: bold; text-decoration: underline;">src/indmanager/ind_manager.c</a> <span style="font-weight: normal;"> (Diff revision 1) </span> </th> </tr> </thead> <tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;"> <tr> <td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool im_stop_ind(IMManager *manager, const CMPIContext *ctx, IMError *err)</pre></td> </tr> </tbody> <tbody> <tr> <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1209</font></th> <td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * If we destroy the thread with mutex locked, the mutex is locked forever.</span></pre></td> </tr> </tbody> </table> <pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I&#39;d just mention, that the mutex must not be locked by the thread we want to cancel. Otherwise both sentences suggest that we want to have the mutex locked forever which does not make sense.</pre> </div> <br /> <p>- Michal Minar</p> <br /> <p>On November 26th, 2013, 11:15 a.m. UTC, Jan Safranek wrote:</p> <table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard-openlmi.rhcloud.com/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;"> <tr> <td> <div>Review request for OpenLMI Developers.</div> <div>By Jan Safranek.</div> <p style="color: grey;"><i>Updated Nov. 26, 2013, 11:15 a.m.</i></p> <div style="margin-top: 1.5em;"> <b style="color: #575012; font-size: 10pt;">Repository: </b> openlmi-providers </div> <h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1> <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0"> <tr> <td> <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">indicationmgr: fixed deadlock when re-starting the namager thread. When the indication manager thread was stopped, it destroys its mutex and creates new one. If pthread_mutex_destroy() fails because the mutex is locked, the new mutex is not created -&gt; the old one is used, but it&#39;s already locked -&gt; deadlock. So let&#39;s make sure the mutex is unlocked when destroying the thread. Rewrite of indication manager thread is needed to have the manage() function interruptible when waiting for events, e.g. using a pipe and select(), this pthread_cancel leads to memory leaks. </pre> </td> </tr> </table> <h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1> <ul style="margin-left: 3em; padding-left: 0;"> <li>src/indmanager/ind_manager.c <span style="color: grey">(7d6bc7545647589178ed2a858c0d136c4806b7b0)</span></li> </ul> <p><a href="http://reviewboard-openlmi.rhcloud.com/r/1291/diff/" style="margin-left: 3em;">View Diff</a></p> </td> </tr> </table> </div> </body> </html> --===============3066450856508988246==-- ]