You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2012/02/03 02:02:08 UTC
svn commit: r1239966 - in /subversion/trunk/subversion/mod_dav_svn:
dav_svn.h mod_dav_svn.c repos.c
Author: stsp
Date: Fri Feb 3 01:02:08 2012
New Revision: 1239966
URL: http://svn.apache.org/viewvc?rev=1239966&view=rev
Log:
Add a mod_dav_svn configuration option to set the environment of hook scripts.
The default environment of hooks is still empty.
The new option is called SVNHooksEnv, applies globally or within
<Location> directives, and takes any number of arguments.
Each argument specifies one environment variable in the form VAR=VAL.
If spaces appear in the variable name or value the entire option argument
needs to be quoted. For example:
SVNHooksEnv LC_CTYPE=en_US.UTF-8 "VAR_WITH_A_SPACE=foo bar"
* subversion/mod_dav_svn/dav_svn.h
(dav_svn__get_hooks_env): Declare.
* subversion/mod_dav_svn/mod_dav_svn.c
(dir_conf_t): Add hooks_env.
(create_dir_config): Initialise hooks_env.
(merge_dir_config): Merge hooks_env fields.
(SVNHooksEnv_cmd): New. Parses arguments given to SVNHooksEnv option
into a hash table.
(dav_svn__get_hooks_env): Returns the hooks environment hash table
parsed from the configuration.
(cmds): Add SVNHooksEnv option.
* subversion/mod_dav_svn/repos.c
(env_from_env_hash): New helper function. Copies an environment from
a hash table into an array of C strings.
(get_resource): If necessary, configure the repository's hooks
environment after opening the repository.
Modified:
subversion/trunk/subversion/mod_dav_svn/dav_svn.h
subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
subversion/trunk/subversion/mod_dav_svn/repos.c
Modified: subversion/trunk/subversion/mod_dav_svn/dav_svn.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/dav_svn.h?rev=1239966&r1=1239965&r2=1239966&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/dav_svn.h (original)
+++ subversion/trunk/subversion/mod_dav_svn/dav_svn.h Fri Feb 3 01:02:08 2012
@@ -385,6 +385,9 @@ const char *dav_svn__get_root_dir(reques
/* Return the data compression level to be used over the wire. */
int dav_svn__get_compression_level(void);
+/* Return the hook script environment parsed from the configuration. */
+apr_hash_t *dav_svn__get_hooks_env(request_rec *r);
+
/** For HTTP protocol v2, these are the new URIs and URI stubs
returned to the client in our OPTIONS response. They all depend
on the 'special uri', which is configurable in httpd.conf. **/
Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=1239966&r1=1239965&r2=1239966&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Fri Feb 3 01:02:08 2012
@@ -25,6 +25,7 @@
#include <stdlib.h>
#include <apr_strings.h>
+#include <apr_hash.h>
#include <httpd.h>
#include <http_config.h>
@@ -94,6 +95,7 @@ typedef struct dir_conf_t {
const char *activities_db; /* path to activities database(s) */
enum conf_flag txdelta_cache; /* whether to enable txdelta caching */
enum conf_flag fulltext_cache; /* whether to enable fulltext caching */
+ apr_hash_t *hooks_env; /* environment for hook scripts */
} dir_conf_t;
@@ -193,6 +195,7 @@ create_dir_config(apr_pool_t *p, char *d
conf->root_dir = svn_urlpath__canonicalize(dir, p);
conf->bulk_updates = CONF_FLAG_ON;
conf->v2_protocol = CONF_FLAG_ON;
+ conf->hooks_env = apr_hash_make(p);
return conf;
}
@@ -222,6 +225,9 @@ merge_dir_config(apr_pool_t *p, void *ba
newconf->txdelta_cache = INHERIT_VALUE(parent, child, txdelta_cache);
newconf->fulltext_cache = INHERIT_VALUE(parent, child, fulltext_cache);
newconf->root_dir = INHERIT_VALUE(parent, child, root_dir);
+ newconf->hooks_env = apr_hash_merge(p, child->hooks_env, parent->hooks_env,
+ NULL /* child overrides parent */,
+ NULL /* unused data baton */);
if (parent->fs_path)
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
@@ -528,6 +534,25 @@ SVNUseUTF8_cmd(cmd_parms *cmd, void *con
return NULL;
}
+static const char *
+SVNHooksEnv_cmd(cmd_parms *cmd, void *config, const char *arg1)
+{
+ apr_array_header_t *var;
+
+ var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
+ if (var && var->nelts == 2)
+ {
+ dir_conf_t *conf = config;
+
+ apr_hash_set(conf->hooks_env,
+ APR_ARRAY_IDX(var, 0, const char *),
+ APR_HASH_KEY_STRING,
+ APR_ARRAY_IDX(var, 1, const char *));
+ }
+
+ return NULL;
+}
+
/** Accessor functions for the module's configuration state **/
@@ -805,6 +830,15 @@ dav_svn__get_compression_level(void)
return svn__compression_level;
}
+apr_hash_t *
+dav_svn__get_hooks_env(request_rec *r)
+{
+ dir_conf_t *conf;
+
+ conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
+ return conf->hooks_env;
+}
+
static void
merge_xml_filter_insert(request_rec *r)
{
@@ -1044,6 +1078,12 @@ static const command_rec cmds[] =
SVNUseUTF8_cmd, NULL,
RSRC_CONF,
"use UTF-8 as native character encoding (default is ASCII)."),
+
+ /* per directory/location */
+ AP_INIT_ITERATE("SVNHooksEnv", SVNHooksEnv_cmd, NULL,
+ ACCESS_CONF|RSRC_CONF,
+ "Set the environment of hook scripts via any number of "
+ "VAR=VAL arguments (the default hook environment is empty)."),
{ NULL }
};
Modified: subversion/trunk/subversion/mod_dav_svn/repos.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/repos.c?rev=1239966&r1=1239965&r2=1239966&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/repos.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/repos.c Fri Feb 3 01:02:08 2012
@@ -1917,7 +1917,32 @@ parse_querystring(request_rec *r, const
return NULL;
}
+/* Copy the environment given as key/value pairs of ENV_HASH into
+ * an array of C strings allocated in RESULT_POOL.
+ * Use SCRATCH_POOL for temporary allocations. */
+static const char **
+env_from_env_hash(apr_hash_t *env_hash,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ apr_hash_index_t *hi;
+ const char **env;
+ const char **envp;
+
+ env = apr_palloc(result_pool,
+ sizeof(const char *) * (apr_hash_count(env_hash) + 1));
+ envp = env;
+ for (hi = apr_hash_first(scratch_pool, env_hash); hi; hi = apr_hash_next(hi))
+ {
+ *envp = apr_psprintf(result_pool, "%s=%s",
+ (const char *)svn__apr_hash_index_key(hi),
+ (const char *)svn__apr_hash_index_val(hi));
+ envp++;
+ }
+ *envp = NULL;
+ return env;
+}
static dav_error *
get_resource(request_rec *r,
@@ -2146,6 +2171,8 @@ get_resource(request_rec *r,
repos->repos = userdata;
if (repos->repos == NULL)
{
+ apr_hash_t *hooks_env;
+
/* construct FS configuration parameters */
fs_config = apr_hash_make(r->connection->pool);
apr_hash_set(fs_config,
@@ -2188,6 +2215,16 @@ get_resource(request_rec *r,
"in repos object",
HTTP_INTERNAL_SERVER_ERROR, r);
}
+
+ /* Configure the hooks environment, if not empty. */
+ hooks_env = dav_svn__get_hooks_env(r);
+ if (hooks_env && apr_hash_count(hooks_env) > 0)
+ {
+ const char **env;
+
+ env = env_from_env_hash(hooks_env, r->connection->pool, r->pool);
+ svn_repos_hooks_setenv(repos->repos, env);
+ }
}
/* cache the filesystem object */
Re: svn commit: r1239966 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c repos.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Feb 03, 2012 at 11:07:39AM +0200, Daniel Shahaf wrote:
> stsp@apache.org wrote on Fri, Feb 03, 2012 at 01:02:08 -0000:
> > +static const char *
> > +SVNHooksEnv_cmd(cmd_parms *cmd, void *config, const char *arg1)
> > +{
> > + apr_array_header_t *var;
> > +
> > + var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> > + if (var && var->nelts == 2)
> > + {
>
> With this code, if an envvar's value legitimately contains a '=', it'll
> be silently skipped. (Example:
> putenv("config-option=servers:global:http-library=serf"))
Ah, you're right. It should require at least two elements and
combine the second element with any that follow.
> > +static const char **
> > +env_from_env_hash(apr_hash_t *env_hash,
> > + apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool)
> > +{
> > + apr_hash_index_t *hi;
> > + const char **env;
> > + const char **envp;
> > +
> > + env = apr_palloc(result_pool,
> > + sizeof(const char *) * (apr_hash_count(env_hash) + 1));
> > + envp = env;
> > + for (hi = apr_hash_first(scratch_pool, env_hash); hi; hi = apr_hash_next(hi))
> > + {
> > + *envp = apr_psprintf(result_pool, "%s=%s",
>
> Heh. So you parse an array of VAR=VAL lines into a hash, and then
> unparse it back? Why not carry it as an array (perhaps an APR array) of
> "VAR=VAL" values?
We need to parse it once anyway to verify the VAR=VAL syntax.
So this was mainly done to make sure we're using the right syntax
for the evecve() env parameter (see e.g. the putenv(3) man page).
After making this change I realised it would be nicer just let the
svn_repos API accept an apr_hash_t and perform the translation to
const char** internally. This would also make it trivial to have
svn_repos API calls to remove or add variables, or merge environments.
Merging might in fact be necessary if we want to support custom hook
environments over all RA protocols. Think about where users could define
custom hook env for file:// access? If it ends up being a config file
in the repository it makes no sense for mod_dav_svn and svnserve to ignore
that file...
Re: svn commit: r1239966 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c repos.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Feb 03, 2012 at 11:07:39AM +0200, Daniel Shahaf wrote:
> stsp@apache.org wrote on Fri, Feb 03, 2012 at 01:02:08 -0000:
> > +static const char *
> > +SVNHooksEnv_cmd(cmd_parms *cmd, void *config, const char *arg1)
> > +{
> > + apr_array_header_t *var;
> > +
> > + var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> > + if (var && var->nelts == 2)
> > + {
>
> With this code, if an envvar's value legitimately contains a '=', it'll
> be silently skipped. (Example:
> putenv("config-option=servers:global:http-library=serf"))
Ah, you're right. It should require at least two elements and
combine the second element with any that follow.
> > +static const char **
> > +env_from_env_hash(apr_hash_t *env_hash,
> > + apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool)
> > +{
> > + apr_hash_index_t *hi;
> > + const char **env;
> > + const char **envp;
> > +
> > + env = apr_palloc(result_pool,
> > + sizeof(const char *) * (apr_hash_count(env_hash) + 1));
> > + envp = env;
> > + for (hi = apr_hash_first(scratch_pool, env_hash); hi; hi = apr_hash_next(hi))
> > + {
> > + *envp = apr_psprintf(result_pool, "%s=%s",
>
> Heh. So you parse an array of VAR=VAL lines into a hash, and then
> unparse it back? Why not carry it as an array (perhaps an APR array) of
> "VAR=VAL" values?
We need to parse it once anyway to verify the VAR=VAL syntax.
So this was mainly done to make sure we're using the right syntax
for the evecve() env parameter (see e.g. the putenv(3) man page).
After making this change I realised it would be nicer just let the
svn_repos API accept an apr_hash_t and perform the translation to
const char** internally. This would also make it trivial to have
svn_repos API calls to remove or add variables, or merge environments.
Merging might in fact be necessary if we want to support custom hook
environments over all RA protocols. Think about where users could define
custom hook env for file:// access? If it ends up being a config file
in the repository it makes no sense for mod_dav_svn and svnserve to ignore
that file...
Re: svn commit: r1239966 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c repos.c
Posted by Daniel Shahaf <da...@elego.de>.
stsp@apache.org wrote on Fri, Feb 03, 2012 at 01:02:08 -0000:
> +static const char *
> +SVNHooksEnv_cmd(cmd_parms *cmd, void *config, const char *arg1)
> +{
> + apr_array_header_t *var;
> +
> + var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> + if (var && var->nelts == 2)
> + {
With this code, if an envvar's value legitimately contains a '=', it'll
be silently skipped. (Example:
putenv("config-option=servers:global:http-library=serf"))
Suggest:
const char *var, *val;
const char *equals = strchr(arg1, '=');
if (equals)
/* edge case: equals == var. What to do? */
var = apr_pstrndup(cmd->pool, arg1, equals - arg1);
else
var = arg1;
if (equals)
val = equals+1;
else
val = "1";
> + dir_conf_t *conf = config;
> +
> + apr_hash_set(conf->hooks_env,
> + APR_ARRAY_IDX(var, 0, const char *),
> + APR_HASH_KEY_STRING,
> + APR_ARRAY_IDX(var, 1, const char *));
> + }
> +
> + return NULL;
> +}
> +
>
> /** Accessor functions for the module's configuration state **/
>
> @@ -805,6 +830,15 @@ dav_svn__get_compression_level(void)
> return svn__compression_level;
> }
>
> +/* Copy the environment given as key/value pairs of ENV_HASH into
> + * an array of C strings allocated in RESULT_POOL.
> + * Use SCRATCH_POOL for temporary allocations. */
> +static const char **
> +env_from_env_hash(apr_hash_t *env_hash,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + apr_hash_index_t *hi;
> + const char **env;
> + const char **envp;
> +
> + env = apr_palloc(result_pool,
> + sizeof(const char *) * (apr_hash_count(env_hash) + 1));
> + envp = env;
> + for (hi = apr_hash_first(scratch_pool, env_hash); hi; hi = apr_hash_next(hi))
> + {
> + *envp = apr_psprintf(result_pool, "%s=%s",
Heh. So you parse an array of VAR=VAL lines into a hash, and then
unparse it back? Why not carry it as an array (perhaps an APR array) of
"VAR=VAL" values?
(And yes, if someone does SVNHooksEnv "X=1" "X=2" and then gets X=1 in
their hooks, they really have only themselves to blame; but I still
wonder why we need the double-translation.)
> + (const char *)svn__apr_hash_index_key(hi),
> + (const char *)svn__apr_hash_index_val(hi));
> + envp++;
> + }
> + *envp = NULL;
>
> + return env;
> +}
>
> static dav_error *
> get_resource(request_rec *r,
Cheers,
Daniel
Re: svn commit: r1239966 - in
/subversion/trunk/subversion/mod_dav_svn: dav_svn.h mod_dav_svn.c repos.c
Posted by Daniel Shahaf <da...@elego.de>.
stsp@apache.org wrote on Fri, Feb 03, 2012 at 01:02:08 -0000:
> +static const char *
> +SVNHooksEnv_cmd(cmd_parms *cmd, void *config, const char *arg1)
> +{
> + apr_array_header_t *var;
> +
> + var = svn_cstring_split(arg1, "=", TRUE, cmd->pool);
> + if (var && var->nelts == 2)
> + {
With this code, if an envvar's value legitimately contains a '=', it'll
be silently skipped. (Example:
putenv("config-option=servers:global:http-library=serf"))
Suggest:
const char *var, *val;
const char *equals = strchr(arg1, '=');
if (equals)
/* edge case: equals == var. What to do? */
var = apr_pstrndup(cmd->pool, arg1, equals - arg1);
else
var = arg1;
if (equals)
val = equals+1;
else
val = "1";
> + dir_conf_t *conf = config;
> +
> + apr_hash_set(conf->hooks_env,
> + APR_ARRAY_IDX(var, 0, const char *),
> + APR_HASH_KEY_STRING,
> + APR_ARRAY_IDX(var, 1, const char *));
> + }
> +
> + return NULL;
> +}
> +
>
> /** Accessor functions for the module's configuration state **/
>
> @@ -805,6 +830,15 @@ dav_svn__get_compression_level(void)
> return svn__compression_level;
> }
>
> +/* Copy the environment given as key/value pairs of ENV_HASH into
> + * an array of C strings allocated in RESULT_POOL.
> + * Use SCRATCH_POOL for temporary allocations. */
> +static const char **
> +env_from_env_hash(apr_hash_t *env_hash,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + apr_hash_index_t *hi;
> + const char **env;
> + const char **envp;
> +
> + env = apr_palloc(result_pool,
> + sizeof(const char *) * (apr_hash_count(env_hash) + 1));
> + envp = env;
> + for (hi = apr_hash_first(scratch_pool, env_hash); hi; hi = apr_hash_next(hi))
> + {
> + *envp = apr_psprintf(result_pool, "%s=%s",
Heh. So you parse an array of VAR=VAL lines into a hash, and then
unparse it back? Why not carry it as an array (perhaps an APR array) of
"VAR=VAL" values?
(And yes, if someone does SVNHooksEnv "X=1" "X=2" and then gets X=1 in
their hooks, they really have only themselves to blame; but I still
wonder why we need the double-translation.)
> + (const char *)svn__apr_hash_index_key(hi),
> + (const char *)svn__apr_hash_index_val(hi));
> + envp++;
> + }
> + *envp = NULL;
>
> + return env;
> +}
>
> static dav_error *
> get_resource(request_rec *r,
Cheers,
Daniel