-----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.
> OK, I pretty much rewrote the whole thing to be
tevent_req-stylish
> with
> _send _recv etc. I agree that it's way more readable now.
Much better, we are close, but not there yet.
0. It is absolutely paramount that you always have:
state structure
_Send()
step1_done()
step2_done()
step3_done()
...
_recv()
(snip very useful explanation)
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()
1. You are trying to use one common state structure for all calls.
Please don't do that, and especially don't add more function pointer
(member_dn_fn is really a no go).
All operations now have a separate struct foo_state.
2. Really don't like add_to_groups() for a few reasons.
- it is creating a subreq so it *should* be a _send() function and
*must* return the subreq and must not set the callback itself.
- this function and the following should probably be turned into a
_send(),_done(),_recv() subrequest.
Done.
- it uses member_dn_fn() please don't do that.
Undone.
3. In tevent_req() _done() functions try to never do something like:
return foo_function(req);
- You should either call tevent_req_error() or tevent_req_done(), or
call a subrequests: subreq = foo_send() and set a callback and then just
return;
- a return foo_function(req) is a spy that something is not right (there
are exceptions, but they must be carefully thought out.
I know some of the functions (like user_mod_cont()) seem to save a bit
of duplication and makes the code slightly smaller, but this is an
optimization that kills readability and doesn't really save much.
Readability is much more important when it comes to tevent_req stuff.
No more user_mod_cont in the current patchset
>> Patch 3:
>> - looks sane but I'd like a second look from one of ours python
> resident
>> experts
>>
>
> CC-ed John.
I see John replied to that part, I'll let you address the concerns he
raised for the 3rd patch.
Simo.
John's concerns should be addressed in the attached patch.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkquhA4ACgkQHsardTLnvCVoQACgwuClflP1QrckaZQdBrl8WUEn
kNsAmwYB5Af85nUvG0U9iEn61sP4nn5+
=YhdE
-----END PGP SIGNATURE-----