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:
>>>> bl...@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?)
> 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.
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)
>> 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 */
SVN_ERR__TRACED
"non-tracing link" /* end of chain */
In this case, the middle link would be lost. Right?
> void
> svn_error_clear(svn_error_t *err)
> {
> if (err)
> {
> #if defined(SVN_DEBUG)
> while (err->child)
> err = err->child;
> apr_pool_cleanup_kill(err->pool, err, err_abort);
> #endif
> svn_pool_destroy(err->pool);
> }
> }
>
> Blair
