Greg Hudson wrote:
> * Use two spaces after periods in the log messages.
> * I think we normally use "err", not "error", to name variables or
> fields containing errors, but I might be making up the consistency of
> that convention.
> * You missed space-before-paren in a number of places.
Corrected.
> * Your design of authz file validation is a little dissatisfactory.
> By performing the checking in authz_group_contains_user() and using that
> function in authz_validate_group(), you've presented a structural
> obstacle to taking advantage of the validation by not doing
> error-checking during actual authz lookup.
> * In fact, if the second patch were designed properly, the first patch
> would be essentially unnecessary.
This design follows up on a question I'd asked on the IRC channel, about
wether this error checking should take place both at load and lookup, or
just load. I was told to check during both (by sussman if my memory is
correct), and so did so. In that frame of implementation, the use of
authz_group_contains_user() during validation is simply a small
factoring to avoid another new function in authz.c .
Now, with the change of data type, it makes more sense to separate
validation and lookup, given that the opaque representation doesn't
allow changing the authz rules after loading, there is no need to
validate twice. I've changed that, validation now happens during
loading, and the lookup is without checking (though errors can still be
thrown).
> + svn_config_get (b->config, &val, "groups",
> + &name[1], NULL);
>
> Fits nicely on one line. Why split it?
No reason, it probably wrapped at some point while I was working on it,
and I didn't look to set it back later. Corrected.
> typedef struct svn_authz_t svn_authz_t;
> struct svn_authz_t {
> svn_config_t *cfg;
> };
Okay, I'll go with that. I wasn't too happy with the way I was handling
the abstraction. This is better, if a little verbose for the time being.
Here is the new patch which implements this change (separation of
validation and lookup and correction of various errors). I think I've
adressed all the things you pointed out, but I'll admit I checked the
patch hastily, due to being pushed for time. There will probably be a
few things to change on this one :-(
To be perfectly honest, if you (all) have other things to see to, I'd
say to leave it. I'll be back by thursday and will take another look at
this submission. Sorry again for the lower quality of this submission, a
lot of things happened at the same time that slowed me down.
I'm now off hiking in the Vosge mountains (nice :), so I won't be back
until next thursday. Sorry for running off after submitting patches like
this, once I get back I'll be available for longer periods at a time.
So, take your time to review if there are other things you want/need to
see to :-)
- Dave.
[[[
Make the authz lookup return errors when it runs into an invalid
configuration file during operation. Add a function which opens and
verifies that an authz configuration file does not contain any logic
errors.
* subversion/include/svn_error_codes.h: New error. Update copyright
notice.
* subversion/include/svn_repos.h
(svn_authz_t): New opaque data type.
(svn_repos_authz_read): New public API.
* subversion/libsvn_repos/authz.c
Use svn_authz_t instead of svn_config_t for handles to authz
configurations.
(authz_baton): Add a svn_error_t* to throw errors out of enumeration
functions.
(authz_group_contains_user_internal,
authz_group_contains_user,
authz_get_path_access,
authz_get_tree_access): Change prototype to return errors, add
error checking code. All callers changed.
(authz_parse_line, authz_parse_section): Throw errors and halt
configuration traversal if necessary.
(svn_repos_authz_check_access): Throw errors back to the caller.
(authz_group_walk, authz_validate_rule,
authz_validate_group, authz_validate_section):
New internal functions.
(svn_repos_authz_read): New function.
* 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 Fri Jul 15 21:20:56 2005