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 */
> SVN_ERR__TRACED
> "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?
Blair
Received on 2010-12-22 22:22:54 CET