On Fri, 2012-06-15 at 15:39 -0400, Stephen Gallagher wrote: > On Wed, 2012-05-30 at 16:27 -0400, Dmitri Pal wrote: > > On 05/30/2012 09:08 AM, Simo Sorce wrote: > > > On Wed, 2012-05-30 at 08:23 -0400, Stephen Gallagher wrote: > > >> This patch was submitted by Shantanu Goel in a Trac ticket. Sending to > > >> the list for review. I haven't yet had a chance to dig into it myself > > >> yet, but the concept as described in > > >> https://fedorahosted.org/sssd/ticket/1354 is sound, so I'd like to see > > >> this get cleaned up and included. > > > > Seems like a big effort. Thank you for the patch! > > We would gladly accept it when the style, comment and functional > > concerns are addressed. > > Please do not be offended, it is just the process we rigorously follow. > > We would absolutely delighted to get more contributions in future! > > > The original submitter provided new patches attached to > https://fedorahosted.org/sssd/ticket/1354 which I have attached for > review. I haven't yet reviewed them myself.
I started doing the review today. Short version: I think the approach here is wrong. My initial glance over this code a few weeks ago didn't catch the fundamental problem with it. It's needlessly complex to run a single polling timer to catch idle timeouts. A simpler and easier to follow approach would be to just set a timer during accept_fd_handler() and reset it each time client_fd_handler() is set. If the timer ever fires, we know that this one, specific client has become idle, and we should then free the client context. Any specialized disconnection logic that is needed should be handled in the client context destructor. In short, I think you've added a lot of overkill to the problem here. Also, the use of send() and MSG_NOSIGNAL is Linux-specific, so we're going to need to add a configure check and conditionalize it. It's a worthwhile enhancement (and belongs in a separate patch), but we want to limit (where possible) the number of places that would break compilation on non-Linux systems. Finally, please do not use tabs in your patches. The coding style for SSSD uses four spaces for indentation. I'm going to take your patch and rework it, because it's become very important to get this in ASAP (we're having resource-exhaustion problems in real-world deployments) and I think it's necessary to get this in immediately. I'll make the necessary modifications myself, but I'm still going to give the patch attribution to you, Shantanu. You've definitely saved me a lot of time and effort by investigating all the gotcha points (such as the MSG_NOSIGNAL piece). Thank you very much for your contribution Shantanu. It's a great help and we hope you'll submit more patches in the future!
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel