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!
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?
> This function is meant to consume the error, so I've updated the docs to
> say that it shouldn't be used after it and clear it.
>
> r1051978.
Received on 2010-12-22 19:30:59 CET