On 06/17/2010 08:32 AM, Eugene Indenbom wrote:
>>
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.
Ack.
>>
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.
Still an 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.
Actually, I'm rethinking this. Probably best to leave the existing code
as-is. However, please rename sdap_cli_connect_recv_new() to
sdap_cli_connect_recv_ext() to remain in keeping with our naming
convention elsewhere.
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.
sdap_id_op_connect_req_done() should be renamed. Please don't use the
_done() suffix for functions that aren't receiving a tevent_req
completion. It gets confusing.
I'm also not terribly enamored of this particular _step() process, but I
can't come up with a better approach right now, so I'll let that stand.
I feel like the individual steps should be wrapped in their own
tevent_req sub-request, but I'm not seeing an obvious place to do that.
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.
Patch 0004: Ack
I think we're on the right track here.
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/