From: Stephen Gallagher <sgallagh@redhat.com>
To: Development of the System Security Services Daemon <sssd-devel@lists.fedorahosted.org>
Cc: dpal@redhat.com; Shantanu Goel <sgoel01@yahoo.com>
Sent: Monday, June 18, 2012 9:11 AM
Subject: Re: [SSSD] [PATCH] Add support for terminating idle connections in sssd_nss
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!