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.
Received on 2013-07-09 22:30:24 CEST