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