Re: thread safe

On 04.09.2012 22:24, Stefan Küng wrote:
> Attached is my first draft of the new svn_config_dup() API.
>
> * it does not return an svn_error_t but the duplicate hash directly.
> The only API call that could return an error is svn_config_create()
> but that API also returns SVN_NO_ERROR unconditionally (for now at
> least).
> * no error checking is done for now, will add some checks for failed
> memory allocations later.
> * hash keys are duplicated as well (allocated in the passed pool)
>
> Please have a look at this. I think it's correct, at least my first
> tests show that it works and solves my problem in TSVN with multiple
> threads as well.
>
> be back tomorrow...

Index: subversion/include/svn_config.h
===================================================================
--- subversion/include/svn_config.h (revision 1380737)
+++ subversion/include/svn_config.h (working copy)
@@ -626,6 +626,16 @@
                                 const char *fname,
                                 apr_pool_t *pool);
 
+/** Return a deep copy of the config hash.
+ * @note use this function instead of repeated calls
+ * to svn_config_get_config to avoid reading the config file
+ * over and over.
+ *
+ * @since New in 1.8.
+ */
+apr_hash_t *
+svn_config_dup(apr_hash_t * config, apr_pool_t * pool);

As I said, the return value must be a svn_config_t*.
More comments below (I'll ignore coding style).

 
Index: subversion/libsvn_subr/config.c
===================================================================
--- subversion/libsvn_subr/config.c (revision 1380737)
+++ subversion/libsvn_subr/config.c (working copy)
@@ -1011,3 +1011,88 @@
   sec = apr_hash_get(cfg->sections, cfg->tmp_key->data, APR_HASH_KEY_STRING);
   return sec != NULL;
 }
+
+
+apr_hash_t *
+svn_config_dup(apr_hash_t * config, apr_pool_t * pool)
+{
+ apr_hash_t * rethash;
+ apr_hash_index_t * cidx;
+ apr_hash_index_t * sectidx;
+ apr_hash_index_t * optidx;
+
+ rethash = apr_hash_make(pool);
+ for (cidx = apr_hash_first(pool, config);
+ cidx != NULL;
+ cidx = apr_hash_next(cidx))
+ {
+ const void *ckey = NULL;
+ void *cval = NULL;
+ apr_ssize_t ckeyLength = 0;
+ svn_config_t * c;
+ svn_config_t * newconfig = NULL;
+
+ svn_config_create(&newconfig, FALSE, pool);

Um, what? You return a hash of svn_config_t? None of the
svn_config_* APIs use such a structure.

+
+ apr_hash_this(cidx, &ckey, &ckeyLength, &cval);
+ c = cval;
+
+ newconfig->sections = apr_hash_make(pool);
+ newconfig->pool = pool;
+ newconfig->x_pool = svn_pool_create(pool);
+ newconfig->x_values = c->x_values;
+ newconfig->tmp_key = svn_stringbuf_dup(c->tmp_key, pool);
+ newconfig->tmp_value = svn_stringbuf_dup(c->tmp_value, pool);
+ newconfig->section_names_case_sensitive = c->section_names_case_sensitive;

svn_config_create already creates the hash table, populates
pool and x_pool, and creates the TEMPORARY stringbufs.
In other words, all you have to do is copy x_values and
section_names_case_sensitive, everything else is already done.

+
+ for (sectidx = apr_hash_first(pool, c->sections);
+ sectidx != NULL;
+ sectidx = apr_hash_next(sectidx))
+ {
+ const void *sectkey = NULL;
+ void *sectval = NULL;
+ apr_ssize_t sectkeyLength = 0;
+ cfg_section_t * sect;
+ cfg_section_t * newsec;
+
+ apr_hash_this(sectidx, &sectkey, &sectkeyLength, &sectval);
+ sect = sectval;
+
+ newsec = apr_palloc(pool, sizeof(*newsec));
+ newsec->name = apr_pstrdup(pool, sect->name);
+ newsec->hash_key = apr_pstrdup(pool, sect->hash_key);
+ newsec->options = apr_hash_make(pool);

Suggest you factor this part out of svn_config_set instead
of duplicating the logic here.

+ for (optidx = apr_hash_first(pool, sect->options);
+ optidx != NULL;
+ optidx = apr_hash_next(optidx))
+ {
+ const void *optkey = NULL;
+ void *optval = NULL;
+ apr_ssize_t optkeyLength = 0;
+ cfg_option_t * opt;
+ cfg_option_t * newopt;
+
+ apr_hash_this(optidx, &optkey, &optkeyLength, &optval);
+ opt = optval;
+
+ newopt = apr_palloc(pool, sizeof(*newopt));
+ newopt->name = apr_pstrdup(pool, opt->name);
+ newopt->hash_key = apr_pstrdup(pool, opt->hash_key);
+ newopt->value = apr_pstrdup(pool, opt->value);
+ newopt->x_value = apr_pstrdup(pool, opt->x_value);
+ newopt->expanded = opt->expanded;
+ apr_hash_set(newsec->options,
+ apr_pstrdup(pool, (const char*)optkey),
+ optkeyLength, newopt);

And here; svn_config_set already has code to create option
and section records, it only needs to be factored out.

+ }
+ apr_hash_set(newconfig->sections,
+ apr_pstrdup(pool, (const char*)sectkey),
+ sectkeyLength, newsec);
+ }
+ apr_hash_set(rethash,
+ apr_pstrdup(pool, (const char*)ckey),
+ ckeyLength, newconfig);
+ }
+
+ return rethash;
+}

So except for the rethash which I frankly don't understand at all, the
rest looks like a good start. Basically just strip away the outer loop
and rely on (refactored) existing code instead of copying it around, and
you're done.

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download