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 2013/10/16 00:32:45 UTC
svn commit: r1532572 - in /subversion/trunk/subversion:
include/private/svn_subr_private.h libsvn_repos/config_pool.c
libsvn_subr/config.c libsvn_subr/config_file.c libsvn_subr/config_impl.h
Author: stefan2
Date: Tue Oct 15 22:32:44 2013
New Revision: 1532572
URL: http://svn.apache.org/r1532572
Log:
Add support for read-only access to svn_config_t. In read-only mode,
concurrent multi-threaded access to the same config data structure is
safe.
We introduce a simple private API function that allows us to switch
a config struct into r/o mode (one-way as the reversal seems dangerous).
Part of that transition is expanding all values to prevent internal
state change upon later read access.
The code expects r/o structs not to be written to and simply ignores
write ops in release mode. In debug mode, an assert()ion is triggered.
This is because the existing setter functions don't provide error
return values.
* subversion/libsvn_subr/config_impl.h
(svn_config_t): add r/o flag to structure
* subversion/include/private/svn_subr_private.h
(void svn_config__set_read_only): declare new private API function
* subversion/libsvn_subr/config_file.c
(expand_value,
expand_values_in_section): new transition utilities
(svn_config__set_read_only): implement new API
* subversion/libsvn_subr/config.c
(svn_config_create2): init new flag as well
(make_string_from_option): assert that this gets never called in r/o mode
(svn_config_set): in r/o mode no-op in release, assert() in debug
* subversion/libsvn_repos/config_pool.c
(expand_value,
expand_values_in_section,
expand_all_values): drop obsolete functions
(auto_parse): simply call new API to ensure correct r/o behavior
Modified:
subversion/trunk/subversion/include/private/svn_subr_private.h
subversion/trunk/subversion/libsvn_repos/config_pool.c
subversion/trunk/subversion/libsvn_subr/config.c
subversion/trunk/subversion/libsvn_subr/config_file.c
subversion/trunk/subversion/libsvn_subr/config_impl.h
Modified: subversion/trunk/subversion/include/private/svn_subr_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_subr_private.h?rev=1532572&r1=1532571&r2=1532572&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_subr_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_subr_private.h Tue Oct 15 22:32:44 2013
@@ -463,6 +463,17 @@ svn_root_pools__release_pool(apr_pool_t
/** @} */
+/**
+ * @defgroup svn_config_private Private configuration handling API
+ * @{
+ */
+
+/* Future attempts to modify CFG will trigger an assertion. */
+void svn_config__set_read_only(svn_config_t *cfg,
+ apr_pool_t *scratch_pool);
+
+/** @} */
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Modified: subversion/trunk/subversion/libsvn_repos/config_pool.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/config_pool.c?rev=1532572&r1=1532571&r2=1532572&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/config_pool.c (original)
+++ subversion/trunk/subversion/libsvn_repos/config_pool.c Tue Oct 15 22:32:44 2013
@@ -147,39 +147,6 @@ struct svn_repos__config_pool_t
};
-/* Callback for svn_config_enumerate2: Continue to next value. */
-static svn_boolean_t
-expand_value(const char *name,
- const char *value,
- void *baton,
- apr_pool_t *pool)
-{
- return TRUE;
-}
-
-/* Callback for svn_config_enumerate_sections2:
- * Enumerate and implicitly expand all values in this section.
- */
-static svn_boolean_t
-expand_values_in_section(const char *name,
- void *baton,
- apr_pool_t *pool)
-{
- svn_config_t *cfg = baton;
- svn_config_enumerate2(cfg, name, expand_value, NULL, pool);
-
- return TRUE;
-}
-
-/* Expand all values in all sections of CONFIG.
- */
-static void
-expand_all_values(config_ref_t *config)
-{
- svn_config_enumerate_sections2(config->cfg, expand_values_in_section,
- config->cfg, config->pool);
-}
-
/* Destructor function for the whole config pool.
*/
static apr_status_t
@@ -401,8 +368,8 @@ auto_parse(svn_config_t **cfg,
svn_stream_from_stringbuf(contents, scratch_pool),
TRUE, TRUE, cfg_pool));
- /* make sure r/o access to config data will not modify the internal state */
- expand_all_values(config_ref);
+ /* switch config data to r/o mode to guarantee thread-safe access */
+ svn_config__set_read_only(config_ref->cfg, cfg_pool);
/* add config in pool, handle loads races and return the right config */
SVN_MUTEX__WITH_LOCK(config_pool->mutex,
Modified: subversion/trunk/subversion/libsvn_subr/config.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/config.c?rev=1532572&r1=1532571&r2=1532572&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/config.c (original)
+++ subversion/trunk/subversion/libsvn_subr/config.c Tue Oct 15 22:32:44 2013
@@ -23,6 +23,8 @@
+#include <assert.h>
+
#define APR_WANT_STRFUNC
#define APR_WANT_MEMFUNC
#include <apr_want.h>
@@ -93,6 +95,7 @@ svn_config_create2(svn_config_t **cfgp,
cfg->tmp_value = svn_stringbuf_create_empty(result_pool);
cfg->section_names_case_sensitive = section_names_case_sensitive;
cfg->option_names_case_sensitive = option_names_case_sensitive;
+ cfg->read_only = FALSE;
*cfgp = cfg;
return SVN_NO_ERROR;
@@ -484,7 +487,13 @@ make_string_from_option(const char **val
*/
if (opt->value && strchr(opt->value, '%'))
{
- apr_pool_t *tmp_pool = (x_pool ? x_pool : svn_pool_create(cfg->x_pool));
+ apr_pool_t *tmp_pool;
+
+ /* setting read-only mode should have expanded all values
+ * automatically. */
+ assert(!cfg->read_only);
+
+ tmp_pool = (x_pool ? x_pool : svn_pool_create(cfg->x_pool));
expand_option_value(cfg, section, opt->value, &opt->x_value, tmp_pool);
opt->expanded = TRUE;
@@ -687,6 +696,15 @@ svn_config_set(svn_config_t *cfg,
cfg_section_t *sec;
cfg_option_t *opt;
+ /* Ignore write attempts to r/o configurations.
+ *
+ * Since we should never try to modify r/o data, trigger an assertion
+ * in debug mode.
+ */
+ assert(!cfg->read_only);
+ if (cfg->read_only)
+ return;
+
remove_expansions(cfg);
opt = find_option(cfg, section, option, &sec);
Modified: subversion/trunk/subversion/libsvn_subr/config_file.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/config_file.c?rev=1532572&r1=1532571&r2=1532572&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/config_file.c (original)
+++ subversion/trunk/subversion/libsvn_subr/config_file.c Tue Oct 15 22:32:44 2013
@@ -37,6 +37,7 @@
#include "svn_ctype.h"
#include "svn_private_config.h"
+#include "private/svn_subr_private.h"
#ifdef __HAIKU__
# include <FindDirectory.h>
@@ -418,9 +419,48 @@ svn_config__sys_config_path(const char *
return SVN_NO_ERROR;
}
+/* Callback for svn_config_enumerate2: Continue to next value. */
+static svn_boolean_t
+expand_value(const char *name,
+ const char *value,
+ void *baton,
+ apr_pool_t *pool)
+{
+ return TRUE;
+}
+
+/* Callback for svn_config_enumerate_sections2:
+ * Enumerate and implicitly expand all values in this section.
+ */
+static svn_boolean_t
+expand_values_in_section(const char *name,
+ void *baton,
+ apr_pool_t *pool)
+{
+ svn_config_t *cfg = baton;
+ svn_config_enumerate2(cfg, name, expand_value, NULL, pool);
+
+ return TRUE;
+}
+
/*** Exported interfaces. ***/
+void
+svn_config__set_read_only(svn_config_t *cfg,
+ apr_pool_t *scratch_pool)
+{
+ /* expand all items such that later calls to getters won't need to
+ * change internal state */
+ svn_config_enumerate_sections2(cfg, expand_values_in_section,
+ cfg, scratch_pool);
+
+ /* now, any modification attempt will be ignored / trigger an assertion
+ * in debug mode */
+ cfg->read_only = TRUE;
+}
+
+
svn_error_t *
svn_config__parse_file(svn_config_t *cfg, const char *file,
Modified: subversion/trunk/subversion/libsvn_subr/config_impl.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/config_impl.h?rev=1532572&r1=1532571&r2=1532572&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/config_impl.h (original)
+++ subversion/trunk/subversion/libsvn_subr/config_impl.h Tue Oct 15 22:32:44 2013
@@ -70,8 +70,11 @@ struct svn_config_t
/* Specifies whether option names are populated case sensitively. */
svn_boolean_t option_names_case_sensitive;
-};
+ /* When set, all modification attempts will be ignored.
+ * In debug mode, we will trigger an assertion. */
+ svn_boolean_t read_only;
+};
/* Read sections and options from a file. */
svn_error_t *svn_config__parse_file(svn_config_t *cfg,
Re: svn commit: r1532572 - in /subversion/trunk/subversion:
include/private/svn_subr_private.h libsvn_repos/config_pool.c
libsvn_subr/config.c libsvn_subr/config_file.c libsvn_subr/config_impl.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Oct 21, 2013 at 7:25 AM, Branko Čibej <br...@wandisco.com> wrote:
> On 20.10.2013 23:26, Stefan Fuhrmann wrote:
> > On Fri, Oct 18, 2013 at 3:36 PM, Branko Čibej <brane@wandisco.com
> > <ma...@wandisco.com>> wrote:
> >
> > On 16.10.2013 00:32, stefan2@apache.org
> > <ma...@apache.org> wrote:
> >> Author: stefan2
> >> Date: Tue Oct 15 22:32:44 2013
> >> New Revision: 1532572
> >>
> >> URL: http://svn.apache.org/r1532572
> >> Log:
> >> Add support for read-only access to svn_config_t. In read-only
> mode,
> >> concurrent multi-threaded access to the same config data structure
> is
> >> safe.
> >
> >> + /* Ignore write attempts to r/o configurations.
> >> + *
> >> + * Since we should never try to modify r/o data, trigger an
> assertion
> >> + * in debug mode.
> >> + */
> >> + assert(!cfg->read_only);
> >> + if (cfg->read_only)
> >> + return;
> >> +
> >> remove_expansions(cfg);
> >
> > Please don't use assert like this. You're assuming that what
> > people like to call "release" builds are always compiled with
> > -DNDEBUG. I've always found that assumption to be naïve at best.
> >
> > Instead, make the code depend on whether we're in maintainer mode
> > or not; the result will be much less ambiguous.
> >
> >
> > From what I can see, there is no macro to test for
> > (SVN_DEBUG being more or less equivalent to DEBUG).
> >
> > Should we introduce SVN_MAINTAINER_MODE?
>
> I'd vote for
>
> #ifdef SVN_DEBUG
> SVN_ERR_ASSERT_NO_RETURN(!cfg->read_only)
> #endif
>
> It looks like the same as "#ifndef NDEBUG", but the important difference
> is that we control it independently of NDEBUG, and more importantly, we
> already use it for these kinds of checks.
>
Committed as r1534110.
-- Stefan^2.
Re: svn commit: r1532572 - in /subversion/trunk/subversion: include/private/svn_subr_private.h
libsvn_repos/config_pool.c libsvn_subr/config.c libsvn_subr/config_file.c
libsvn_subr/config_impl.h
Posted by Branko Čibej <br...@wandisco.com>.
On 20.10.2013 23:26, Stefan Fuhrmann wrote:
> On Fri, Oct 18, 2013 at 3:36 PM, Branko Čibej <brane@wandisco.com
> <ma...@wandisco.com>> wrote:
>
> On 16.10.2013 00:32, stefan2@apache.org
> <ma...@apache.org> wrote:
>> Author: stefan2
>> Date: Tue Oct 15 22:32:44 2013
>> New Revision: 1532572
>>
>> URL: http://svn.apache.org/r1532572
>> Log:
>> Add support for read-only access to svn_config_t. In read-only mode,
>> concurrent multi-threaded access to the same config data structure is
>> safe.
>
>> + /* Ignore write attempts to r/o configurations.
>> + *
>> + * Since we should never try to modify r/o data, trigger an assertion
>> + * in debug mode.
>> + */
>> + assert(!cfg->read_only);
>> + if (cfg->read_only)
>> + return;
>> +
>> remove_expansions(cfg);
>
> Please don't use assert like this. You're assuming that what
> people like to call "release" builds are always compiled with
> -DNDEBUG. I've always found that assumption to be naïve at best.
>
> Instead, make the code depend on whether we're in maintainer mode
> or not; the result will be much less ambiguous.
>
>
> From what I can see, there is no macro to test for
> (SVN_DEBUG being more or less equivalent to DEBUG).
>
> Should we introduce SVN_MAINTAINER_MODE?
I'd vote for
#ifdef SVN_DEBUG
SVN_ERR_ASSERT_NO_RETURN(!cfg->read_only)
#endif
It looks like the same as "#ifndef NDEBUG", but the important difference
is that we control it independently of NDEBUG, and more importantly, we
already use it for these kinds of checks.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1532572 - in /subversion/trunk/subversion:
include/private/svn_subr_private.h libsvn_repos/config_pool.c
libsvn_subr/config.c libsvn_subr/config_file.c libsvn_subr/config_impl.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Oct 18, 2013 at 3:36 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 16.10.2013 00:32, stefan2@apache.org wrote:
>
> Author: stefan2
> Date: Tue Oct 15 22:32:44 2013
> New Revision: 1532572
>
> URL: http://svn.apache.org/r1532572
> Log:
> Add support for read-only access to svn_config_t. In read-only mode,
> concurrent multi-threaded access to the same config data structure is
> safe.
>
>
> + /* Ignore write attempts to r/o configurations.
> + *
> + * Since we should never try to modify r/o data, trigger an assertion
> + * in debug mode.
> + */
> + assert(!cfg->read_only);
> + if (cfg->read_only)
> + return;
> +
> remove_expansions(cfg);
>
>
> Please don't use assert like this. You're assuming that what people like
> to call "release" builds are always compiled with -DNDEBUG. I've always
> found that assumption to be naïve at best.
>
> Instead, make the code depend on whether we're in maintainer mode or not;
> the result will be much less ambiguous.
>
>From what I can see, there is no macro to test for
(SVN_DEBUG being more or less equivalent to DEBUG).
Should we introduce SVN_MAINTAINER_MODE?
-- Stefan^2.
Re: svn commit: r1532572 - in /subversion/trunk/subversion: include/private/svn_subr_private.h
libsvn_repos/config_pool.c libsvn_subr/config.c libsvn_subr/config_file.c
libsvn_subr/config_impl.h
Posted by Branko Čibej <br...@wandisco.com>.
On 16.10.2013 00:32, stefan2@apache.org wrote:
> Author: stefan2
> Date: Tue Oct 15 22:32:44 2013
> New Revision: 1532572
>
> URL: http://svn.apache.org/r1532572
> Log:
> Add support for read-only access to svn_config_t. In read-only mode,
> concurrent multi-threaded access to the same config data structure is
> safe.
> + /* Ignore write attempts to r/o configurations.
> + *
> + * Since we should never try to modify r/o data, trigger an assertion
> + * in debug mode.
> + */
> + assert(!cfg->read_only);
> + if (cfg->read_only)
> + return;
> +
> remove_expansions(cfg);
Please don't use assert like this. You're assuming that what people like
to call "release" builds are always compiled with -DNDEBUG. I've always
found that assumption to be naïve at best.
Instead, make the code depend on whether we're in maintainer mode or
not; the result will be much less ambiguous.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com