[SSSD] LDAP connection tracking, sharing and fail-over retry framework

Eugene Indenbom eindenbom at gmail.com
Wed Jul 7 08:06:53 UTC 2010


On 07/06/2010 11:17 PM, Stephen Gallagher wrote:
> <...>
>
>>      0005-Use-new-LDAP-connection-framework-to-get-user-accoun.patch
>>      
> Nitpick: in users_connect_get_done()
> +    int ret, dp_error = DP_ERR_FATAL;
>
> Please either use:
> int ret, dp_error;
> or
> int ret;
> int dp_error = DP_ERR_FATAL;
>
> It's easier to read.
>    
Agreed.
> However, since dp_error is assigned in the very next line (in a way that
> can't fail) I don't see a good reason to initialize it here.
>
>    
It's a bit complicated question and generally matter of coding style. 
What can we assume about output parameters of functions defined in the 
other source file?

My opinion is that we should always initialize output parameters in the 
calling method, just to be on the safe side. What is your opinion?
>>      0006-Use-new-LDAP-connection-framework-to-get-group-accou.patch
>>      
> Same nitpick as patch 5.
>
>    
>>      0007-Use-new-LDAP-connection-framework-to-get-user-accoun.patch
>>      
> Same nitpick as patch 5.
>
>    
> <...>
>>      0010-Use-new-LDAP-connection-framework-in-IPA-access-back.patch
>>      
> Nack.
>
> Using hbac_ctx->sdap_op == NULL to mean "operating in offline mode" is
> very hard to read. Please just set a boolean state like
> hbac_ctx->offline instead. When I read "if (!hbac_ctx->sdap_op)", my
> natural expectation is for the following lines to set up the sdap_op.
>
> I'm not finished reviewing this patch. This may take a bit more time.
>    
The problem here is that we get 2 interrelated fields in hbac_ctx and 
will have to add asserts testing in online mode that hbac_ctx->sdap_op 
is not NULL.
The same applies to the prototypes of HBAC stages:
     hbac_get_host_info_send()
     hbac_get_rules_send()
     hbac_get_service_data_send()

We will have to pass both hbac_ctx->offline and hbac_ctx->sdap_op as 
parameters with the same problem as above.

 From my point of view, the solution could be to pass the above methods 
hbac_ctx itself and define hbac_ctx_is_offline() test. This way the code 
will be readable and no duplicate info will be kept in data structures.


>    
>>      0011-Use-new-LDAP-connection-framework-in-IPA-dynamic-DNS.patch
>>      
> Nack.
>
> There are several problems with this patch:
>
> 1) When the dynamic DNS update code was written, it expected that when
> we are online, we would always have an active LDAP connection. This is
> no longer guaranteed to be true. As we have discussed previously, it
> will be possible with this new interface (eventually) to be able to
> configure it so that connections to LDAP are not maintained after the
> currently-running set of operations completes. So it's possible that it
> will not be available to sdap_id_get_cached_connection() here. This
> function needs to be able to create a connection when necessary (so
> we'll need to convert part of the dynamic DNS update code to expect an
> async operation at this point.
>    
Well, actually I have modelled the implementation after current approach 
that can currently even segfault if connection gets dropped during 30ms 
delay. Another reason is that ldap/ipa backend ONLINE callbacks are only 
called when LDAP connection is established, so it does make sense to 
assume that connection is established.

Please, let me know your opinion and I will add async establishment of 
LDAP connection if needed.
> 2) sdap_id_op_connect_cached() is never used anywhere. As mentioned
> above, we should ask the connection cache if there is an available
> connection and using it if so, creating a new connection if not.
>
>    
I have added this method for the sake of completeness. It can be safely 
removed. Currently I do not see any use for it.




More information about the sssd-devel mailing list