Comments inline below. This is a purely code-inspection review at the
moment. Functional testing will be performed once we get the code in shape.
On 06/16/2010 02:43 AM, Eugene Indenbom wrote:
The patches attached provide a framework for LDAP connection
tracking,
sharing and fail-over retries in LDAP ID backend, which is designed to
address the tickets #468, 464, 465 and 466.
The patches are structured as follows:
0001-GSSAPI-ticket-expiry-time-is-returned-from-ldap_chil.patch
A small utility patch saving GSSAPI ticket expiry time in sdap_handle
for future use.
You need to pass &expire_time into ldap_child_get_tgt_sync(). Right now
you're passing a time_t instead of a time_t *
Minor nitpicks: We prefer that pointers be initialized to NULL, rather
than 0 explicitly. In parse_child_response you initialize to zero. Also,
talloc_free() is NULL-aware, so it's unnecessary to wrap it in an if
(ccn != 0) {} block. Finally, I don't see the need for the goto cleanup
here. There is no other way to jump here, so it makes more sense to me
to just call talloc_free() inside the error handling block and return
from there.
0002-Added-an-interface-to-query-number-of-configured-and.patch
Another utility patch allowing to dynamically determine number of
fail-over servers (including resolved through SRV records)
Looks straightforward. Ack.
0003-LDAP-connection-usage-tracking-sharing-and-failover-.patch
The main patch -- LDAP connection tracking, sharing and fail-over retry
framework
sdap_cli_connect_recv_new() is unnecessary. It should replace
sdap_cli_connect_recv() (and retain the old name). Also, change the
sdap_cli_connect_recv_new() function to check for NULL before assigning
to can_retry (making it an optional return). Then this function is just
a drop-in replacement for anywhere it's currently used, just adding a
NULL argument.
If sdap_id_release_conn_data() is called by
sdap_id_conn_cache_be_offline_cb() and returns without releasing the
connection (because of active operations), we probably need to set a
flag on the conn_data and continue trying to release it at the end of
each operation (until the last operation finally can do so).
Could you please explain the comment "we clean out list of ops to make
sure that order of destruction does not matter" in
sdap_id_conn_data_destroy()? I'm not sure what effect you think this is
having on the destruction.
Also, sdap_id_conn_data_destroy() is not a properly constructed talloc
destructor (I'm not sure why it's compiling without a warning). The
argument needs to be of type 'TALLOC_CTX *ctx' and then you should do
struct sdap_id_op *op = talloc_get_type(ctx, struct sdap_id_op). You
will also need to cast to (TALLOC_CTX *) in the destructor
initialization. Same for sdap_id_op_destroy().
Please add more parentheses in the comparison in
sdap_is_connection_expired(). I can tell what the intended order of
operations is, but I'm not 100% sure that it will end up being the
actual order :)
sdap_id_conn_data_set_expire_timer() does not properly set the timeval.
Use tevent_timeval_current_ofs() to set the timeval instead. Tevent uses
explicit times, not offsets, so what you're doing here is guaranteeing
that the event will always be fired immediately (unless your system
clock happens to be running close to the epoch), not expire_time seconds
in the future.
The DLIST_REMOVE() in sdap_id_op_hook_conn_data() has a NULL dereference
error if op is NULL. You need to check for valid input to the function
before this step. (Preferably with a DEBUG message if it is not valid)
sdap_id_op_can_reconnect() Rather than special-case two retries here, we
should probably just mark the server as unusable when the connection is
broken and move to the next failover server. The slim chance that the
server will be available after a reconnect is not worth the overhead of
waiting for a connection timeout.
sdap_id_opt_connect() is not following proper tevent_req behavior at
all. First of all, it should be named sdap_id_opt_connect_send() and
must have a return type of tevent_req *. On failure, this function must
return NULL (and may also return an error code variable by reference).
The _send function itself MUST create the connection request.
Pre-creating it to be used when this function is called is unsafe at any
speed. Also, the tevent_req MUST be returned on a success condition,
even if reusing an existing operation connection. For this purpose,
tevent_req_post() should be used to guarantee that the results fire
immediately after the calling function returns control to the mainloop
(in the case where we are reusing the connection. Never use
tevent_req_post() except in a _send() function). Any processing based on
results from this request should be handled in the corresponding _done()
function. Short-circuiting it in the original calling function (as is
done in users_get_retry in Patch 0004) is unsafe (as it duplicates code
and makes it harder to keep track of).
sdap_id_op_connect_retry() should be converted to
sdap_id_opt_connect_step() and should be a tevent_req *subreq function.
Once it returns a valid connection (or has determined that no such
connection CAN be made) it should call tevent_req_done() or
tevent_req_error() and return control to sdap_id_opt_connect_done().
Follow the style set out by sdap_cli_connect_send() and _done() and
_recv() here.
If you have questions on how to approach this, please ask. I know I've
given only a high-level reply here.
Minor nitpicks:
The change to the prototype for users_get_recv() belongs in patch 0004.
Also, our coding style is to use 'int *var' not 'int* var' (which I
notice you have been following elsewhere, so I assume this is just a
typo) You did the same in sdap_id_conn_cache_create() for the conn_cache
variable.
While it's not strictly required by C, we prefer for function prototypes
to list the same variable names as the function implementation. static
int sdap_id_op_destroy(struct sdap_id_op*); does not do this.
0004-Use-new-LDAP-connection-framework-to-get-user-accoun.patch
Sample usage of the new LDAP connection framework for user account
information request
See comments above regarding the proper tevent_req style for
sdap_id_opt_connect_send() and sdap_id_opt_connect_recv().
Please note that the patches are provided only as a material for
comments and discussion on solution architecture. I have only made sure
that they compile. I am completely open to suggestions and other
approaches on how to solve the above problems.
I like the general concept, but we're going to need a few iterations of
the code to get it into shape. Keep up the good work!
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/