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" <stsp_at_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.
Received on 2013-07-12 13:55:53 CEST