Greg Hudson wrote:
> *cfgp should be *authzp.
> cfg should have been changed to authz.
Fixed.
> As far as I can tell, you still have the cyclic dependency from the
> first case in these rules, making this a fragile test.
Oops. Well spotted. In my defense, the tests still succeed when the
right thing is tested consistently :-).
> Why didn't you leave this returning a boolean? You seem to have left
> in some of the internal structure for returning errors from
> svn_repos_authz_check_access(), which is unnecessary gunk in the
> current implementation since it never actually returns an error.
> Perhaps you reasoned that the internals of authz.c should leave the
> door open to returning errors in the future, in which case I do not
> agree. While I think the API should leave the door open to that
> possibility, any good implementation will not return errors; thus,
> the internal logic does not need to contain gunk to that end.
Okay. You are right, I'd left error throwing in the internal
implementation in prevision of possible future error throwing, which
is useless for the time being.
> I advise reverting the function signature here, reverting the
> addition of the err field to the baton, reverting the call site in
> authz_parse_line(), reverting the signature of authz_get_tree_access
> and the changes to that function, reverting the signature of
> authz_get_path_access and the changes to that function, and reverting
> the call sites to the two get_*_access functions in
> svn_repos_authz_check_access().
Done. As the error field is still necessary to the validation
functions, I've split authz_baton into authz_lookup_baton and
authz_validation_baton.
> The rest of the patch (the validation checking) looks great. One
> more round and I think we should be good.
Hopefully you're right. Thanks for reviewing.
- Dave.
[[[
Add a function which opens and verifies that an authz configuration
file does not contain any logic errors prior to any lookups.
* subversion/include/svn_error_codes.h: New error. Update copyright
notice.
* subversion/include/svn_repos.h: Remove useless include.
(svn_authz_t): New opaque data type.
(svn_repos_authz_read): New public API.
(svn_repos_authz_check_access): Fix docstring typo. Use a
svn_authz_t* instead of a svn_config_t*.
* subversion/libsvn_repos/authz.c
Use svn_authz_t instead of svn_config_t for handles to authz
configurations.
(authz_baton): Rename.
(authz_lookup_baton): Renamed from authz_baton. All references
changed.
(authz_validate_baton): New baton for use in validation routines.
(authz_group_contains_user_internal): Remove function.
(authz_group_contains_user): Remove useless protection against
invalid configurations.
(authz_get_path_access): Rewrite docstring.
(authz_get_tree_access): Return the access status directly.
(authz_group_walk, authz_validate_rule,
authz_validate_group, authz_validate_section):
New internal functions.
(svn_repos_authz_read): New function.
(svn_repos_authz_check_access): Change calls to altered internal
functions.
* subversion/tests/libsvn_repos/repos-test.c
Use svn_authz_t instead of svn_config_t for handles to authz
configurations.
(authz_get_handle): New function. Factor out the converting a char*
authz file contents to a parsed svn_authz_t* from the authz test
function.
(authz): Add a second batch of tests to ensure that invalid authz
configurations are caught during the authz loading.
* subversion/mod_authz_svn/mod_authz_svn.c
Use svn_authz_t instead of svn_config_t for handles to authz
configurations.
(req_check_access): replace call to svn_config_read with a call to
svn_repos_authz_read.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 23 18:41:27 2005