Re: thread safe

On 05.09.2012 15:24, Stefan Küng wrote:
> 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.

Ah, I begin to understand; you want to duplicate the result of
svn_config_get_config, instead of the result of
svn_config_create/svn_config_read. I tend to think these are two
separate functions:

svn_error_t *svn_config_dup(svn_config_t **cfgp, svn_config_t *src, apr_pool_t *pool);
svn_error_t *svn_config_copy_config(apr_hash_t **cfg_hash, apr_hash_t *src_hash, apr_pool_t *pool);

Sorry about the misunderstanding.

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

I'm not suggesting you use svn_config_set, that would be wrong, because
it fiddles with value expansions. I'm suggesting that you factor out the
parts of svn_config_set that create section and options into separate
static helper functions, so that the new code and svn_config_set can
both use those. That's better than duplicating the code.

-- Brane

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