On Tue, Jul 09, 2013 at 12:46:36AM +0800, 刘新星 wrote: > What I am doing is to add ldap group support for subversion. Which means we > don't need any predefined groups in authz file. We get groups from ldap > server directly.
Hello, thank you very much for this patch! I have some questions and suggestions. Are you aware of mod_ldap? http://httpd.apache.org/docs/2.2/mod/mod_ldap.html This is a module that provides ldap services to other Apache HTTPD modules. If mod_authz_svn was using mod_ldap it it could achieve better performance due to caching provided by mod_ldap. You are linking libsvn_subr to the openldap library to support ldap queries. This allows both mod_authz_svn and svnserve to access the ldap server, which is good. I believe we should add ldap support to both svnserve and mod_authz_svn, so a new dependency on openldap seems inevitable, at least for svnserve. If you made use of mod_ldap in mod_authz_svn, could the new ldap.c file be moved from libsvn_subr to svnserve somehow? Then we could avoid indirectly linking every subversion program to openldap via libsvn_subr. Such an approach might also require some svn_repos API changes to make data obtained from ldap accessible to libsvn_repos. But it might be worth considering regardless. In your patch, you extended svn_config_t and moved it to a public API header. I think it would be better to keep svn_config_t private. You could add new ldap-specific data to svn_authz_t and keep it private, too. Provide svn_repos__authz_ API functions to get/set the data in svn_authz_t if necessary. Then you don't need to make the definition of svn_authz_t public. The configuration variable names you're adding to mod_authz_svn conflict with variable names used by mod_authnz_ldap. Like AuthLDAPBindPassword for example, see here: http://httpd.apache.org/docs/2.2/mod/mod_authnz_ldap.html#authldapbindpassword Please use names like SVNAuthLDAPBindPassword instead to avoid conflicts. There are several sections in your patch that do not conform to our coding style guidelines described here: http://subversion.apache.org/docs/community-guide/conventions.html#coding-style Perhaps you can spot them? If not, we'll point them out in another review :) That's all for now. I hope you will send a revised version of the patch.
