Greg Hudson wrote:
> * build.conf currently only makes mod_authz_svn depend on libsvn_subr.
> You'll need to change that to make it depend on libsvn_repos, I believe.
Done.
> * Your addition to repos-test.c opens a file /tmp/authz.tmp, which is
> not portable to Windows and is probably a security issue on Unix. You
> should be using apr_file_mktemp(); see tests/libsvn_delta/random-test.c
> for an example.
Ouch. I meant to ask about that, but it slipped my mind.
As I'm using svn_config_read(), which requires a file path, I create the
temp file with apr_file_mktemp() write the contents using
apr_file_write_full(), then get the path to read back using
apr_file_name_get(). The temp file is closed (and deleted) as soon as
the readback is complete. Is this satisfactory?
> * I think we generally put two spaces after sentence-ending periods in
> log messages and doc strings. (Except when it's the end of a comment,
> in which case one space before the "*/" is fine.) You're only putting
> one space after them.
Fixed. Unless I missed it, this isn't in HACKING. Should it be?
> * What kind of testing have you done with mod_authz_svn in particular,
> to make sure that I won't break it when I commit this?
As I couldn't find any test framework for mod_authz_svn, I simply loaded
the trunk mod_dav_svn and mod_authz_svn into my Apache2 and manually
queried a greek tree repository in a way to trigger each of the authz
test cases that are tested in repos-test.c. The results were
constistent with those given by the authz test framework, as were the
other authz lookups that were triggered as side effects of the ones I
was aiming for. The authz logs are also identical to those produced by
a 1.2.0 build operating on the same repository/ACLs.
I don't really see what else I could test here, but I'm open to
suggestion on how to go about testing mod_authz_svn in an automated way.
Speaking of tests, I've added comments in the authz test function to
explain what aspect of the authz behaviour I'm testing in each case
(recursion, fallback to parent paths, use of pan-repository rules...).
> "Check whether". You mispell "whether" in five other places in the
> patch as well.
Oops! I was convinced that was spelled 'wether'. Corrected.
> [snip]
I've reviewed my comments and corrected the problems you found.
Here goes again!
[[[
Move authz routines from mod_authz_svn to libsvn_repos; rework authz
code to fit in with libsvn_repos; update mod_authz_svn to reflect
the move.
* subversion/mod_authz_svn/mod_authz_svn.c: Add includes, remove
nameless enum. Update copyright notice.
(parse_authz_baton, group_contains_user_internal,
group_contains_user, parse_authz_line,
parse_authz_lines, parse_authz_section,
parse_authz_sections, check_access): Remove, functionality
moves to libsvn_repos.
(req_check_access): Use the authz routines in libsvn_repos and wrap
any errors reported by the authz layer. Test for a NULL repos_path
here rather than in check_access.
* subversion/include/svn_repos.h: Add include. Update copyright notice.
(svn_repos_authz_access_t): New enum type.
(svn_repos_authz_check_access): New public API.
* subversion/libsvn_repos/authz.c: New file. Contains the functionality
moved from mod_authz_svn. Rename functions and variable names to
better fit libsvn_repos. Add internal API and in-function
documentation.
(authz_access_is_granted, authz_access_is_determined): New functions
that refactor and document obfuscated tests that were previously in
authz_parse_section and authz_get_path_access.
* subversion/tests/libsvn_repos/repos-test.c: Add include. Update
copyright notice.
(authz): New test.
* build.conf: Add dependancy on libsvn_repos to mod_authz_svn build
rules.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 3 19:13:07 2005