<html><body><div style="color:#000; background-color:#fff; font-family:times new roman, new york, times, serif;font-size:12pt"><div><span>Hi Stephen,</span></div><div><br><span></span></div><div><span>Please feel free to modify the patch in any way or shape you deem necessary for inclusion.&nbsp; We are just glad that you agree there is a real problem which needs fixing.&nbsp; One thing I ask is if you expect to have rhel 5 or 6 test RPMs that we could test with the ultimate fix any time soon, please drop me a note and we will gladly install them on some of our problematic machines here to see if they address the problems we have seen.</span></div><div><br><span></span></div><div><span>Regards,</span></div><div><span>Shantanu<br></span></div><div><br></div>  <div style="font-family: times new roman, new york, times, serif; font-size: 12pt;"> <div style="font-family: times new roman, new york, times, serif; font-size: 12pt;"> <div dir="ltr"> <font
 face="Arial" size="2"> <hr size="1">  <b><span style="font-weight:bold;">From:</span></b> Stephen Gallagher &lt;sgallagh@redhat.com&gt;<br> <b><span style="font-weight: bold;">To:</span></b> Development of the System Security Services Daemon &lt;sssd-devel@lists.fedorahosted.org&gt; <br><b><span style="font-weight: bold;">Cc:</span></b> dpal@redhat.com; Shantanu Goel &lt;sgoel01@yahoo.com&gt; <br> <b><span style="font-weight: bold;">Sent:</span></b> Monday, June 18, 2012 9:11 AM<br> <b><span style="font-weight: bold;">Subject:</span></b> Re: [SSSD] [PATCH] Add support for terminating idle connections in sssd_nss<br> </font> </div> <br>
On Fri, 2012-06-15 at 15:39 -0400, Stephen Gallagher wrote:<br>&gt; On Wed, 2012-05-30 at 16:27 -0400, Dmitri Pal wrote:<br>&gt; &gt; On 05/30/2012 09:08 AM, Simo Sorce wrote:<br>&gt; &gt; &gt; On Wed, 2012-05-30 at 08:23 -0400, Stephen Gallagher wrote:<br>&gt; &gt; &gt;&gt; This patch was submitted by Shantanu Goel in a Trac ticket. Sending to<br>&gt; &gt; &gt;&gt; the list for review. I haven't yet had a chance to dig into it myself<br>&gt; &gt; &gt;&gt; yet, but the concept as described in<br>&gt; &gt; &gt;&gt; <a href="https://fedorahosted.org/sssd/ticket/1354" target="_blank">https://fedorahosted.org/sssd/ticket/1354</a> is sound, so I'd like to see<br>&gt; &gt; &gt;&gt; this get cleaned up and included.<br>&gt; &gt; <br>&gt; &gt; Seems like a big effort. Thank you for the patch!<br>&gt; &gt; We would gladly accept it when the style, comment and functional<br>&gt; &gt; concerns are addressed.<br>&gt; &gt; Please do not be offended, it is just the
 process we rigorously follow.<br>&gt; &gt; We would absolutely delighted to get more contributions in future!<br>&gt; <br>&gt; <br>&gt; The original submitter provided new patches attached to<br>&gt; <a href="https://fedorahosted.org/sssd/ticket/1354" target="_blank">https://fedorahosted.org/sssd/ticket/1354</a> which I have attached for<br>&gt; review. I haven't yet reviewed them myself.<br><br><br>I started doing the review today.<br><br>Short version: I think the approach here is wrong. My initial glance<br>over this code a few weeks ago didn't catch the fundamental problem with<br>it.<br><br>It's needlessly complex to run a single polling timer to catch idle<br>timeouts. A simpler and easier to follow approach would be to just set a<br>timer during accept_fd_handler() and reset it each time<br>client_fd_handler() is set. If the timer ever fires, we know that this<br>one, specific client has become idle, and we should then free the
 client<br>context.<br><br>Any specialized disconnection logic that is needed should be handled in<br>the client context destructor. In short, I think you've added a lot of<br>overkill to the problem here.<br><br>Also, the use of send() and MSG_NOSIGNAL is Linux-specific, so we're<br>going to need to add a configure check and conditionalize it. It's a<br>worthwhile enhancement (and belongs in a separate patch), but we want to<br>limit (where possible) the number of places that would break compilation<br>on non-Linux systems.<br><br>Finally, please do not use tabs in your patches. The coding style for<br>SSSD uses four spaces for indentation.<br><br>I'm going to take your patch and rework it, because it's become very<br>important to get this in ASAP (we're having resource-exhaustion problems<br>in real-world deployments) and I think it's necessary to get this in<br>immediately.<br><br>I'll make the necessary modifications myself, but I'm still going
 to<br>give the patch attribution to you, Shantanu. You've definitely saved me<br>a lot of time and effort by investigating all the gotcha points (such as<br>the MSG_NOSIGNAL piece).<br><br>Thank you very much for your contribution Shantanu. It's a great help<br>and we hope you'll submit more patches in the future!<br><br><br> </div> </div>  </div></body></html>