Thank you for your suggestions!
mod_ldap for performance improvement is a further consideration. Actually I want to add ldap support for mod_authz_svn most. As far as I can see, when we need ldap authentication, we would take these combination 'apache + subversioin'. Is it a good idea to move ldap.c from libsvn_subr to mod_authz_svn ? Further more, I will make ldap group support an optional feature when building. As to variable names, I am considering just reuse the variable names of mod_authnz_ldap. If we define these variable names for both mod_authnz_ldap and mod_authz_svn for the same value, I don't think it is a good idea. What do you think? 在 2013-07-10 04:29:42，"Stefan Sperling" <s...@elego.de> 写道： >On Tue, Jul 09, 2013 at 12:46:36AM +0800, 刘新星 wrote: >> What I amy 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.
