Re: thread safe

On Wed, Sep 5, 2012 at 1:14 AM, Branko Čibej <brane_at_wandisco.com> wrote:
> 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).

Why must it be an svn_config_t* ?
That makes no sense: we want to duplicate the config hash.

>
> 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.

Yes, the config hash that you get from
svn_config_get_config().
And the config hash you have to pass to client APIs as part of the
client context object.

>
> +
> + 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.

ok, thanks for the hint.

> +
> + 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.

Yes, but I don't like the idea of using svn_config_set to do this
because it does more than is necessary here.
We don't have to check whether to replace or add an entry because we
always add (we're copying).

And the question is: do we want an exact copy or a new object,
initialized with the same data?

>
> + }
> + 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.

as I said, the hash is necessary because that's the object we want to
duplicate, the object that's passed to svn client APIs.
To deal with configs alone, we don't really need a duplicate function.

Stefan

-- 
       ___
  oo  // \\      "De Chelonian Mobile"
 (_,\/ \_/ \     TortoiseSVN
   \ \_/_\_/>    The coolest Interface to (Sub)Version Control
   /_/   \_\     http://tortoisesvn.net