subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From stef...@apache.org
Subject 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
Date Tue, 15 Oct 2013 22:32:45 GMT
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,



Mime
View raw message