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
> be reversed
^^^ this is still not addressed, once you do 0001 can be acked.
> - also some cases where codyng style is not followed (missing
> between 'if' and '('
Should be done. Note that the patch is different than the original one
b/c of the removal of shadow-utils fallback.
The rest looks ok.
> Patch 2: NACK
> - do not create tools/common/ keep all in tools/ until we have a
> of files to get out of the way
> - main NACK point is that tevent_req async coding style has not been
> followed in the sync wrappers, details explained on IRC
OK, I pretty much rewrote the whole thing to be tevent_req-stylish
_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:
In this order, if you have to jump around to find out any step the code
becomes very soon unreadable and it is extremely easy to make mistakes.
This code is already somewhat harder than usual to debug (need to set a
lof of break points to follow the flow) so it is imperative that you can
read it as a single set of consecutive functions.
The importance of this is immediately evident whan you have to A) review
some else's code (like I am doing now), B) you have to modify come code.
Especially important is to share only utility functions but never
functions that create tevent_req requests.
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).
When you share the same state structure a typo later in the code where
you set (for example) a wrong callback (say a copy&paste error where you
call the xx_users_recv() instead of the xx_groups_recv() will give back
no error. If you use different state functions though, you can use
talloc_get_type() to check for the type, and NULL will be returned if
the type is wrong resulting in a segfault the first time you test it.
Also remember that tevent_req_create() does not zero out the memory (on
purpose it's not a bug), so anything you do not initialize explicitly in
the _send() function is random.
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
- it uses member_dn_fn() please don't do that.
3. In tevent_req() _done() functions try to never do something like:
- 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
- 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.
> - also do the transaction as a separate operation and simply
> handle to the sync ones, so that multiple sync operations can be
> into a single transaction.
Transactions are now done outside the "sync" module, that is, either
the tools or in the python bindings module.
> Patch 3:
> - looks sane but I'd like a second look from one of ours python
I see John replied to that part, I'll let you address the concerns he
raised for the 3rd patch.
Simo Sorce * Red Hat, Inc * New York