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()?
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.
> 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.
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
Received on 2010-12-22 19:45:55 CET