[PATCH] clean the event context after fork in fork_domain_child()

[Jeremy Allison]

On Wed, Jan 07, 2009 at 05:39:17PM +0100, Stefan (metze) Metzmacher wrote:
> boyang schrieb:
> > Stefan (metze) Metzmacher wrote:
> >> boyang schrieb:
> >>   
> >>> Jeremy Allison wrote:
> >>>     
> >>>> On Tue, Jan 06, 2009 at 01:19:41PM +0800, boyang wrote:
> >>>>   
> >>>>       
> >>>>> Jeremy Allison wrote:
> >>>>>     
> >>>>>         
> >>>> Ok, I've finished doing a lot of cleanup work on the
> >>>> event code in winbindd. It's all checked into 3.3 and
> >>>> master. Please review.
> >>>>
> >>>> If you feel it's working, feel free to back-port to 3.2
> >>>> and/or 3.0.x and I'll commit patches for you. I'm not
> >>>> doing that work yet as I'm not sure if we're doing any
> >>>> more 3.2.x releases or just moving on to 3.3.0.
> >>>>   
> >>>>       
> >>> Hi, Jeremy:
> >>>      Your patch is good. I have done some initial test on it. And I
> >>> think we can do some work more to clean the context.
> >>> 1*  _client_list or winbindd_client_list() is never used in child,
> >>> destroy and free it.
> >>> 2* file descriptors come from listen socket in parent, accepted socket
> >>> in parent, socket pairs in parent are never used, close it and delete is
> >>> from fd_events.
> >>> 3* delete all memory credentials too as newly forked child won't use it.
> >>> I did some initial test and it works.
> >>> Patches are in the attachment, please review it. Thanks!
> >>>     
> >>>> Jeremy.
> >>>>
> >>>>   
> >>>>       
> >> Why do you use the deprecated talloc_destroy() ?
> >>   
> > Hi, Jeremy && metze:
> >        This is the updated version of the patch.
> > 1* Got rid of deprecated talloc_destroy() and use TALLOC_FREE().
> > 2* do a test to determine if there is  opened child->event.fd.
> > Pleas review it. Thanks!
> >> metze
> 
> having a close_winbindd_socket_after_fork() just as a one function
> should be avoided.
> 
> It would be nice if you would format the code like this:
> 
> /*                     <-- note this line
>  * Long Comment for
>  * step2 ....
>  */                    <-- and this
> step1_fn1();
> step1_var=1;
>                        <-- and this
> /* Short Comment for step 2 */
> step2_var=2;
> step2_fn();
> 
> 
> It makes the code much more readable (at least for me:-)
> 
> Regarding the functionality I trust Jeremy.

Ok, I hate to be negative here, but I don't think this
patch is needed. The original set of patches were
essential as they caused actual bugs, in that the
events were left active and could trigger inappropriately.

This code tidies things up, but except for the fd
closing doesn't really fix any bugs or resource leaks.

Back-porting the original fixes to 3.2 or 3.0 is
probably more useful (IMHO).

Jeremy.