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:
- 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.
- 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.