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

Jakub Hrozek jhrozek at redhat.com
Wed Apr 24 18:57:57 UTC 2013


On Tue, Apr 23, 2013 at 02:02:17PM -0400, Simo Sorce wrote:
> 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.

Yes, there is a unit test in the DNS update patches plus I did some
manual sanity testing.

Thank you for the review, I will push the patch to master.



More information about the sssd-devel mailing list