On Mon, 2009-09-14 at 19:57 +0200, Jakub Hrozek wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Thank you for the review, new patches attached.
>
> On 09/13/2009 04:20 PM, Simo Sorce wrote:
> > On Thu, 2009-09-10 at 23:36 +0200, Jakub Hrozek wrote:
> >>> Patch 1:
> >>> - mostly good but mem hierarchy between ops_ctx and tools_ctx needs
> >> to
> >>> be reversed
> >
> > ^^^ this is still not addressed, once you do 0001 can be acked.
> >
>
> Sorry, but I'm confused now. During the first review, you said that:
> "you should make tctx a child of octx so that talloc_free(octx) will be
> enough"
> The code now allocates octx first, then passes it as mem_ctx parameter
> to setup_db where it is used to create tctx.
this is the code I see for the function
init_sss_tools()
+ octx = talloc_zero(tctx, struct ops_ctx);
+ if (octx == NULL) {
+ DEBUG(1, ("Could not allocate memory for data context\n"));
+ ERROR("Out of memory\n");
+ ret = ENOMEM;
+ goto fini;
+ }
+ octx->ctx = tctx;
+
+ *_octx = octx;
This means that the 'octx' memory context is the child of 'tctx'.
Needs to be the other way around otherwise if you free octx, tctx will
not be automatically freed.
> Hope it is better now, see i.e:
>
> struct user_mod_state
> user_mod_send()
> user_mod_attr_done()
> user_mod_rm_group_done()
> user_mod_add_group_done()
[..]
> All operations now have a separate struct foo_state.
> > _send(),_done(),_recv() subrequest.
> Done.
[..]
> > - it uses member_dn_fn() please don't do that.
> Undone.
[..]
> No more user_mod_cont in the current patchset
Awesome, this latest code looks exactly as I was thinking it should be.
I'll test it asap.
> >>> Patch 3:
> >>> - looks sane but I'd like a second look from one of ours python
> >> resident
> >>> experts
> > I see John replied to that part, I'll let you address the concerns he
> > raised for the 3rd patch.
> John's concerns should be addressed in the attached patch.
I'll let John comment on it
Simo.
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
