Blair Zajac wrote on Wed, Dec 22, 2010 at 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:
>>>>>> 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?)
>
> 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.
Hmm, right. I read make_error_internal() too fast and missed that.
> 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().
>
Agreed. As to the 'lost' links (those not pointed to any more),
I believe they're allocated from the same pool as every other link in
the chain, so they'll be freed when that chain is cleared.
>>> 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.
>
IOW, duplicating in maintainer mode forces duplicating in non-maintainer
mode too, making the common case costlier. True.
However, we could have a macro that causes the dup() operation to only
be done in maintainer mode:
void svn_error_purge_tracing_shell(svn_error_t **err_p)
{
#ifdef SVN_ERR__TRACING
svn_error_t *err = svn_error_purge_tracing(*err_p);
svn_error_clear(*err_p);
*err_p = err;
#endif /* SVN_ERR__TRACING */
}
> 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.
>
IOW, change its promise such that it's the passed-in error, not the
returned one, which should be cleared? And have it leave the passed-in
chain untouched?
It makes sense on a first scan, but at 3am it's too late for a full +1.
>> 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.
>
Thanks for the correction.
>>>> 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?
>
Won't the inner while() loop would skip over them?
> Blair
