Re: [SSSD] [PATCH] Do not keep growing event context

[Simo Sorce]

On Mon, 2013-04-22 at 12:18 +0200, Jakub Hrozek wrote:
>  On Fri, Apr 19, 2013 at 02:14:21PM -0400, Simo Sorce wrote:
>  > > > > >      /* Stop monitoring for this child */
>  > > > > >      talloc_free(child_ctx);
>  > > > > > +    talloc_free(imm);
>  > > > > >  }
>  > > > > 
>  > > > > Why not simply make the imm context a child of the child_ctx it is
> freed
>  > > > > just over here ?
>  > > > > 
>  > > > > Why is it made child of the event context at all ?
>  > > > > 
>  > > > > Simo.
>  > > > 
>  > > > I will check the code again but I think it was because we wanted to be
>  > > > on the safe side and make sure the context is still valid even in case
>  > > > of timeouts etc. so we allocated it on the event context which is always
>  > > > available and long lived.
>  > > > 
>  > > > I'll check and change the code if appropriate.
>  > > 
>  > > I didn't find any reason the immediate context could outlive the
>  > > child_ctx so I allocated the immediate context on the child_ctx.
>  > > 
>  > > But in the case of the sss_child_invoke_cb() I think it is still better to
>  > > free the handler there because the caller owns the child_ctx and I don't
>  > > think we should grow it over time. In the case of child_invoke_callback
>  > > the child_ctx is released there, so we're good.
>  > > 
>  > > New patch is attached.
>  > 
>  > Looks good to me.
>  > 
>  > Simo.
>  
>  Can I treat this as an Ack? :-) 
If you are confident of your testing, then yes.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York