[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

From: Blair Zajac <blair_at_orcaware.com>
Date: Wed, 22 Dec 2010 13:22:11 -0800

On 12/22/10 10:56 AM, Daniel Shahaf wrote:
> Blair Zajac wrote on Wed, Dec 22, 2010 at 10:45:12 -0800:
>> On 12/22/10 10:27 AM, Daniel Shahaf wrote:
>>> Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:
>>>> On 12/22/10 5:47 AM, Daniel Shahaf wrote:
>>>>> blair_at_apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -0000:
>>>>>> Author: blair
>>>>>> Date: Wed Dec 22 05:46:45 2010
>>>>>> New Revision: 1051763
>>>>>> URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
>>>>>> Log:
>>>>>> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
>>>>>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
>>>>>> ==============================================================================
>>>>>> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
>>>>>> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
>>>>>> @@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
>>>>>> name, value, pool);
>>>>>> }
>>>>>> +const char *
>>>>>> +svn_repos__post_commit_error_str(svn_error_t *err,
>>>>>> + apr_pool_t *pool)
>>>>>> +{
>>>>>> + svn_error_t *hook_err1, *hook_err2;
>>>>>> +
>>>>>> + if (! err)
>>>>>> + return "(no error)";
>>>>>> +
>>>>>> + err = svn_error_purge_tracing(err);
>>>>> This modifies the provided error. Either the doc string should warn
>>>>> that this is being done (i.e., the provided ERR is not safe for use
>>>>> after calling this helper), or this call should be removed and another
>>>>> means used to locate the first non-tracing error message.
>>>>> Where do you clear ERR? You must do so in this function, since the
>>>>> caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
>>>>> promise).
>>>> Is this promise a defensive promise, as the original err still looks
>>>> usable by reviewing svn_error_purge_tracing()'s implementation? It looks
>>>> like only one of the errors should ever be cleared and it doesn't matter
>>>> which one. True, it's conceptually easier to remember that one should
>>>> only clear the error returned from svn_error_purge_tracing().
>>> I hadn't actually examined the implementation at the time I wrote that
>>> email, so now I'm confused.
>>> svn_error_purge_tracing() returns a chain which is a subset of the
>>> original chain. (The subset need not start at the same place as the
>>> original chain, eg in the common case that the first entry in the
>>> original chain is a tracing link.)
>>> The reason for clearing errors is to free() the memory and deinstall the
>>> err_abort() pool cleanup callback. Since svn_error_purge_tracing()
>>> doesn't free() or deinstall the callback, it would seem that one has to
>>> clear the ORIGINAL (passed-in) error chain, not the returned one!
>> Even this isn't guarenteed to work, if the error at the end of the chain
>> is a tracing link, right, since it can be removed by
>> svn_error_purge_tracing()?
> Ah, right. Once you call svn_error_purge_tracing(), if the chain
> contains a tracing link at any place other than the first position, then
> all pointers to that link would be lost.
> (But if this is indeed a bug, then how come it hasn't been noticed yet?)

I think it only matters if the last error in the chain is a tracing error,
because the removal of err_abort() from the pool has to use the exact values
that were used to register err_abort(), which is the last error in the chain.
Since probably 100% of the time the last error in a chain is not a tracing
error, then err_abort() will be properly deregistered by svn_error_clear().

>> It seems the best thing is to have svn_error_purge_tracing() return a
>> brand new chain that shares no links with the original chain, and then
>> both need to be freed.
> That makes sense; if we'll work with complete chains, there's a smaller
> chance of more edge-case bugs.

Thinking on this a little more, since most people won't use debugging, it'll be
wasteful to have svn_error_purge_tracing() make a duplicate chain that will have
no tracing errors in it.

So maybe we should have svn_error_purge_tracing() be related to the input chain
and share what it can, but not be an error chain one should use
svn_error_clear() on.

> Separately, it seems that svn_error_dup() installs a pool cleanup
> callback only on one link in the duplicate chain, rather than on every
> link. (whereas make_error_internal() causes every link of a 'normal'
> chain to have a cleanup attached)

make_error_internal() only installs a callback on the last error in the chain,
that is if "child" is NULL.

>>> For example, shouldn't the following code trigger err_abort() on the
>>> outer (wrapper) link?
>>> svn_error_t *in = svn_error_create(...);
>>> svn_error_t *original = svn_error_quick_wrap(in, SVN_ERR__TRACED);
>>> svn_error_t *purged = svn_error_purge_tracing(original);
>>> svn_error_clear(purged);
>>> exit(0);
>>> Am I making sense?
>> I don't think so, because if SVN_DEBUG is defined, then it finds the
>> error at the end of the chain, which in this example is a non-tracing
>> error, and it'll properly remove the err_abort.
> You're right, I'll modify my example to this:
> SVN_ERR__TRACED /* start of chain */
> "non-tracing link" /* end of chain */
> In this case, the middle link would be lost. Right?

Both SVN_ERR__TRACED would be lost by svn_error_purge_tracing(), right?

Received on 2010-12-22 22:22:54 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.