Roderich Schupp <roderich.sch...@gmail.com> writes:
> The patch needs to #include libsvn_subr/config_impl.h
> in order to gain access to svn_config_t.pool:
> the cache (apr_hash_t itself, keys and values) must be allocated from
> the same pool as svn_config_t so that they have the same lifespan.
It would probably better to introduce a new API something like:
apr_pool_t *svn_config__pool(svn_config_t *);
declared in subversion/include/private/.
However I don't fully understand the logic governing the lifetime. The
cache is part of svn_authz_t so I would expect it's lifetime to be that
of svn_authz_t. If that's the case you add a pool member to svn_authz_t
and use that.
> Index: subversion/libsvn_repos/authz.c
> ===================================================================
> --- subversion/libsvn_repos/authz.c (revision 1508093)
> +++ subversion/libsvn_repos/authz.c (working copy)
> @@ -37,6 +37,9 @@
> #include "private/svn_fspath.h"
> #include "repos.h"
>
> +/* we need just svn_config_t.pool */
> +#include "../libsvn_subr/config_impl.h"
> +
>
> /*** Structures. ***/
>
> @@ -44,7 +47,7 @@
> lookup. */
> struct authz_lookup_baton {
> /* The authz configuration. */
> - svn_config_t *config;
> + svn_authz_t *authz;
>
> /* The user to authorize. */
> const char *user;
> @@ -76,14 +79,24 @@
> enumerator, if any. */
> };
>
> +svn_boolean_t authz_use_cache = TRUE; /* FIXME devel only */
> +
I assume that is temporary, what is the long-term plan? Remove it
completely?
> /* Currently this structure is just a wrapper around a
> svn_config_t. */
That comment is out-of-date.
> struct svn_authz_t
> {
> svn_config_t *cfg;
> + const char *cached_user;
> + apr_hash_t *cache;
Add a comment describing what types are used as keys and values in the
hash.
> };
>
> +struct access_cache_t
> +{
> + svn_repos_authz_access_t allow;
> + svn_repos_authz_access_t deny;
> +};
>
> +
>
> /*** Checking access. ***/
>
> @@ -241,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);
> }
> @@ -295,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 = apr_hash_get(cache, path, APR_HASH_KEY_STRING);
svn_hash_gets
> + if (access)
> + {
> + /* ... combine baton with cached allow/deny and we're done */
> + baton->allow |= access->allow;
> + baton->deny |= access->deny;
> + return;
> + }
> +
> + /* reuse baton: save baton's allow/deny information 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->config->pool instead of pool as the cache has
> + * the same lifetime as baton->config.
> + */
> + access = apr_palloc(cfg->pool, sizeof(*access));
> + access->allow = baton->allow;
> + access->deny = baton->deny;
> + apr_hash_set(cache, apr_pstrdup(cfg->pool, path), APR_HASH_KEY_STRING,
> access);
svn_hash_sets
> +
> + /* combine the result with saved values */
> + 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.
> */
> @@ -310,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,
> @@ -338,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,
> @@ -346,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);
> @@ -364,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);
> @@ -382,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;
> @@ -398,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;
> @@ -420,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);
>
> @@ -440,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. */
> @@ -460,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. */
> @@ -754,6 +820,9 @@
> {
> struct authz_validate_baton baton = { 0 };
>
> + authz->cached_user = NULL; /* FIXME does apr_palloc zero the alloced
> stuff? */
> + authz->cache = NULL; /* FIXME does apr_palloc zero the alloced
> stuff? */
apr_palloc doesn't zero, apr_pcalloc does.
> +
> baton.err = SVN_NO_ERROR;
> baton.config = authz->cfg;
>
> @@ -1027,13 +1096,40 @@
> {
> const char *current_path;
>
> + /* Note: Below we use authz->cfg->pool instead of pool as the cache must
> + * have the same lifetime as authz->cfg.
> + */
> + if (authz_use_cache) /* FIXME devel only */
> + {
> + /* Check whether user is still the same as authz->cached_user;
> + * otherwise blow away authz->cache and remember the new user.
> + */
> + if (!( user && authz->cached_user
> + && strcmp(user, authz->cached_user) == 0 ))
We don't usually put whitespace there
if (!(user && authz->cached_user
&& strcmp(user, authz->cached_user) == 0))
> + {
> + if (user)
> + {
> + authz->cache = NULL;
> + authz->cached_user = apr_pstrdup(authz->cfg->pool, user);
> + }
> + else if (authz->cached_user)
> + {
> + authz->cache = NULL;
> + authz->cached_user = NULL;
> + }
> + }
> +
> + if (!authz->cache)
> + authz->cache = apr_hash_make(authz->cfg->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;
> }
> @@ -1045,11 +1141,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. */
> @@ -1068,7 +1162,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;
>
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
