On 06/16/2010 06:20 PM, Stephen Gallagher wrote:
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 *
Oops, thanks for catching it. I have missed a compiler warning as I am
used to compiling with -Werror option and forgot to do a full rebuild
before sending patches out. Also as I have mentioned before, I haven't
tested the patches after merging from 1.2 code base to master. :(
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.
Done.
> 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).
I have added a new function to
minimize changes. I have planned to
remove old sdap_cli_connect_recv (and rename sdap_cli_connect_recv_new)
when all code will be converted to new framework.
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.
Done.
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).
That's already done. See sdap_id_op_hook_conn_data. Whenever connection
is detached from operation (in sdap_id_op_done, destructor or failed
connection), sdap_id_release_conn_data is called to garbage collect
no-longer-needed connection.
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.
When sdap_id_ctx is being destroyed and there are outstanding LDAP
queries, the order of destruction is unclear (or probably even
undefined): it is possible that BE request (and consequently
corresponding sdap_id_op) will be destroyed before sdap_id_conn_cache.
As both of them have common ancestor, but none of them is child of the
other.
Therefore when sdap_id_conn_data is being destroyed and there is active
sdap_id_op we break the link between them by removing sdap_id_op from
list, but do not call completion callbacks (for example for connection
establishment) as we assume that sdap_id_op is being destroyed anyway.
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().
TALLOC library has changed meanwhile. TALLOC version 2 uses typed
destructors. Just take a look into talloc.h and see how type safety is
implemented using simple macros and modern gcc features.
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 :)
Done.
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.
What I have done looks OK for me. I am setting timeval not to offset,
but first to connection expiration time (absolute time in seconds from
EPOCH):
tv.tv_sec = conn_data->sh->expire_time;
Then I subtract LDAP query timeout from it:
tv.tv_sec -= timeout;
I assume that connection should be dropped from cache if it will expire
before query time-out.
Actually this code was tested on 1.2 code base and works, at least for me.
After looking in tevent.h I wanted to change my code to use
tevent_timeval_set and tevent_timeval_add, but tevent_timeval_set uses
32-bit seconds, which is long obsolete as time_t is 64-bit and there is
no tevent_timeval_sub. :(
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)
Done.
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.
I agree that it's better to move to next server after current one
failed, but it is not acceptable to mark current server as not working
just because of broken connection as there is a good change that it was
a recoverable problem like expired GSSAPI ticket. Unfortunately
fail-over API currently does not have such functionality.
So I would propose to make a separate patch which will add such a
functionality to fail-over and then use it in sdap_id_op_done when
broken connection is detected.
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).
I do not agree that short-circuiting in this case is not worthwhile.
From my point of view, overhead of allocating request and
tevent_request_post should be avoided.
But anyway, changed it to full tevent style compliance.
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.
Done.
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.
Oops. Fixed.
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.
Fixed. Code style in my company is int*. So it's more matter of custom,
but I am trying hard to follow SSSD code style. :)
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.
Done.
> 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().
Done.
The modified patches are attached. As last time the changes were not
tested, just compiled. If they look OK, the next step for me would be to
convert all the remaining code in series of patches and test the result.
Eugene