Hi,
revised patch attached, hopefully addressing all concerns.
No more breaking encapsulation. We simply allocate svn_authz_t from the
same pool
that svn_config_t is allocated from and remember this pool in a new
svn_authz_t
member "pool".l Create a function authz_create() for that; it uses
apr_pcalloc
so that we don't have to explicitly zero members "cache" and "cached_user"
any more.
Cheers, Roderich
Index: subversion/libsvn_repos/authz.c
===================================================================
--- subversion/libsvn_repos/authz.c (revision 1511246)
+++ subversion/libsvn_repos/authz.c (working copy)
@@ -45,7 +45,7 @@
lookup. */
struct authz_lookup_baton {
/* The authz configuration. */
- svn_config_t *config;
+ svn_authz_t *authz;
/* The user to authorize. */
const char *user;
@@ -77,14 +77,26 @@
enumerator, if any. */
};
-/* Currently this structure is just a wrapper around a
- svn_config_t. */
+/* Information needed for authz lookups */
struct svn_authz_t
{
- svn_config_t *cfg;
+ svn_config_t *cfg; /* The configuration file where to look up
+ authorizations. */
+ apr_hash_t *cache; /* Cache of previous lookup results (w.r.t. user
+ CACHED_USER). A hash table whose keys are paths
+ and whose values are access_cache_t *. */
+ const char *cached_user; /* User for which lookups are cached. */
+ apr_pool_t *pool; /* The pool this struct was allocated in. */
};
+/* Cached authz information */
+struct access_cache_t
+{
+ svn_repos_authz_access_t allow;
+ svn_repos_authz_access_t deny;
+};
+
/*** Checking access. ***/
@@ -242,10 +254,10 @@
*/
if (rule_match_string[0] == '@')
return authz_group_contains_user(
- b->config, &rule_match_string[1], b->user, pool);
+ b->authz->cfg, &rule_match_string[1], b->user, pool);
else if (rule_match_string[0] == '&')
return authz_alias_is_user(
- b->config, &rule_match_string[1], b->user, pool);
+ b->authz->cfg, &rule_match_string[1], b->user, pool);
else
return (strcmp(b->user, rule_match_string) == 0);
}
@@ -296,6 +308,62 @@
}
+/* Wrapper for svn_config_enumerate2(..., authz_parse_line, ...)
+ * that manages the allow/deny per-section cache.
+ */
+static void
+authz_lookup_cached(const char *path,
+ struct authz_lookup_baton *baton,
+ apr_pool_t *pool)
+{
+ struct svn_config_t *cfg = baton->authz->cfg;
+ struct apr_hash_t *cache = baton->authz->cache;
+ struct access_cache_t *access;
+ svn_repos_authz_access_t saved_allow, saved_deny;
+
+ if (!cache)
+ {
+ svn_config_enumerate2(cfg, path,
+ authz_parse_line, baton, pool);
+ return;
+ }
+
+ /* If section [path] doesn't exist, then we're done. */
+ if (!svn_config_has_section(cfg, path))
+ return;
+
+ /* If we already know allow/deny for section [path] ... */
+ access = svn_hash_gets(cache, path);
+ if (access)
+ {
+ /* ... combine baton with the cached allow/deny and we're done. */
+ baton->allow |= access->allow;
+ baton->deny |= access->deny;
+ return;
+ }
+
+ /* Reuse baton: save baton's allow/deny and reset it. */
+ saved_allow = baton->allow;
+ saved_deny = baton->deny;
+ baton->allow = baton->deny = svn_authz_none;
+
+ svn_config_enumerate2(cfg, path, authz_parse_line, baton, pool);
+
+ /* Cache the result.
+ * Note: Use baton->authz->pool instead of pool as the cache must have
+ * the same lifetime as baton->authz.
+ */
+ access = apr_palloc(baton->authz->pool, sizeof(*access));
+ access->allow = baton->allow;
+ access->deny = baton->deny;
+ svn_hash_sets(cache, apr_pstrdup(baton->authz->pool, path), access);
+
+ /* Combine the result with the saved allow/deny. */
+ baton->allow |= saved_allow;
+ baton->deny |= saved_deny;
+}
+
+
/* Callback to parse a section and update the authz_baton if the
* section denies access to the subtree the baton describes.
*/
@@ -311,9 +379,8 @@
return TRUE;
/* Work out what this section grants. */
- b->allow = b->deny = 0;
- svn_config_enumerate2(b->config, section_name,
- authz_parse_line, b, pool);
+ b->allow = b->deny = svn_authz_none;
+ authz_lookup_cached(section_name, b, pool);
/* Has the section explicitly determined an access? */
conclusive = authz_access_is_determined(b->allow, b->deny,
@@ -339,7 +406,7 @@
* successfully determined.
*/
static svn_boolean_t
-authz_get_path_access(svn_config_t *cfg, const char *repos_name,
+authz_get_path_access(svn_authz_t *authz, const char *repos_name,
const char *path, const char *user,
svn_repos_authz_access_t required_access,
svn_boolean_t *access_granted,
@@ -347,14 +414,13 @@
{
const char *qualified_path;
struct authz_lookup_baton baton = { 0 };
-
- baton.config = cfg;
+
+ baton.authz = authz;
baton.user = user;
/* Try to locate a repository-specific block first. */
qualified_path = apr_pstrcat(pool, repos_name, ":", path, (char *)NULL);
- svn_config_enumerate2(cfg, qualified_path,
- authz_parse_line, &baton, pool);
+ authz_lookup_cached(qualified_path, &baton, pool);
*access_granted = authz_access_is_granted(baton.allow, baton.deny,
required_access);
@@ -365,7 +431,7 @@
return TRUE;
/* No repository specific rule, try pan-repository rules. */
- svn_config_enumerate2(cfg, path, authz_parse_line, &baton, pool);
+ authz_lookup_cached(path, &baton, pool);
*access_granted = authz_access_is_granted(baton.allow, baton.deny,
required_access);
@@ -383,14 +449,14 @@
* searched, return the updated authorization status.
*/
static svn_boolean_t
-authz_get_tree_access(svn_config_t *cfg, const char *repos_name,
+authz_get_tree_access(svn_authz_t *authz, const char *repos_name,
const char *path, const char *user,
svn_repos_authz_access_t required_access,
apr_pool_t *pool)
{
struct authz_lookup_baton baton = { 0 };
- baton.config = cfg;
+ baton.authz = authz;
baton.user = user;
baton.required_access = required_access;
baton.repos_path = path;
@@ -399,7 +465,7 @@
/* Default to access granted if no rules say otherwise. */
baton.access = TRUE;
- svn_config_enumerate_sections2(cfg, authz_parse_section,
+ svn_config_enumerate_sections2(authz->cfg, authz_parse_section,
&baton, pool);
return baton.access;
@@ -421,9 +487,8 @@
strlen(b->qualified_repos_path)) == 0)
{
b->allow = b->deny = svn_authz_none;
-
- svn_config_enumerate2(b->config, section_name,
- authz_parse_line, baton, pool);
+
+ authz_lookup_cached(section_name, baton, pool);
b->access = authz_access_is_granted(b->allow, b->deny,
b->required_access);
@@ -441,14 +506,14 @@
* to any path within the REPOSITORY. Return TRUE if so. Use POOL
* for temporary allocations. */
static svn_boolean_t
-authz_get_any_access(svn_config_t *cfg, const char *repos_name,
+authz_get_any_access(svn_authz_t *authz, const char *repos_name,
const char *user,
svn_repos_authz_access_t required_access,
apr_pool_t *pool)
{
struct authz_lookup_baton baton = { 0 };
- baton.config = cfg;
+ baton.authz = authz;
baton.user = user;
baton.required_access = required_access;
baton.access = FALSE; /* Deny access by default. */
@@ -461,7 +526,7 @@
* may not always have). So we end up enumerating the sections in
* the authz CFG and stop on the first match with some access for
* this user. */
- svn_config_enumerate_sections2(cfg, authz_get_any_access_parser_cb,
+ svn_config_enumerate_sections2(authz->cfg, authz_get_any_access_parser_cb,
&baton, pool);
/* If walking the configuration was inconclusive, deny access. */
@@ -934,12 +999,21 @@
return SVN_NO_ERROR;
}
+/* Allocate a svn_authz_t in POOL and remember it in svn_authz_t->pool. */
+static svn_authz_t *
+authz_create(apr_pool_t *pool)
+{
+ svn_authz_t *authz = apr_pcalloc(pool, sizeof(*authz));
+ authz->pool = pool;
+ return authz;
+}
+
svn_error_t *
svn_repos__authz_read(svn_authz_t **authz_p, const char *path,
const char *groups_path, svn_boolean_t must_exist,
svn_boolean_t accept_urls, apr_pool_t *pool)
{
- svn_authz_t *authz = apr_palloc(pool, sizeof(*authz));
+ svn_authz_t *authz = authz_create(pool);
/* Load the authz file */
if (accept_urls)
@@ -996,7 +1070,7 @@
svn_repos_authz_parse(svn_authz_t **authz_p, svn_stream_t *stream,
svn_stream_t *groups_stream, apr_pool_t *pool)
{
- svn_authz_t *authz = apr_palloc(pool, sizeof(*authz));
+ svn_authz_t *authz = authz_create(pool);
/* Parse the authz stream */
SVN_ERR(svn_config_parse(&authz->cfg, stream, TRUE, TRUE, pool));
@@ -1028,13 +1102,36 @@
{
const char *current_path;
+ /* Check whether USER is still the same as authz->cached_user;
+ * otherwise blow away authz->cache and remember the new USER.
+ * Create authz->cache if it doesn't exist yet.
+ * Note: Use authz->pool instead of pool as the cache must have the
+ * same lifetime as authz.
+ */
+ if (!(user && authz->cached_user
+ && strcmp(user, authz->cached_user) == 0))
+ {
+ if (user)
+ {
+ authz->cache = NULL;
+ authz->cached_user = apr_pstrdup(authz->pool, user);
+ }
+ else if (authz->cached_user)
+ {
+ authz->cache = NULL;
+ authz->cached_user = NULL;
+ }
+ }
+ if (!authz->cache)
+ authz->cache = apr_hash_make(authz->pool);
+
if (!repos_name)
repos_name = "";
/* If PATH is NULL, check if the user has *any* access. */
if (!path)
{
- *access_granted = authz_get_any_access(authz->cfg, repos_name,
+ *access_granted = authz_get_any_access(authz, repos_name,
user, required_access, pool);
return SVN_NO_ERROR;
}
@@ -1046,11 +1143,9 @@
path = svn_fspath__canonicalize(path, pool);
current_path = path;
- while (!authz_get_path_access(authz->cfg, repos_name,
+ while (!authz_get_path_access(authz, repos_name,
current_path, user,
- required_access,
- access_granted,
- pool))
+ required_access, access_granted, pool))
{
/* Stop if the loop hits the repository root with no
results. */
@@ -1069,7 +1164,7 @@
the entire authz config to see whether any child paths are denied
to the requested user. */
if (*access_granted && (required_access & svn_authz_recursive))
- *access_granted = authz_get_tree_access(authz->cfg, repos_name, path,
+ *access_granted = authz_get_tree_access(authz, repos_name, path,
user, required_access, pool);
return SVN_NO_ERROR;
