You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/12/22 18:56:48 UTC
Re: svn_error_purge_tracing() and clearing the errors Re: svn
commit: r1051763 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_repos/commit.c
mod_dav_svn/version.c
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@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
How to recognize that an SVN_ERR_ASSERT() triggered Re:
svn_error_purge_tracing() and clearing the errors Re: svn commit:
r1051763 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_repos/commit.c
mod_dav_svn/version.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Fri, Dec 24, 2010 at 10:48:24 +0200:
> Blair Zajac wrote on Thu, Dec 23, 2010 at 17:09:31 -0800:
> > On 12/23/10 3:01 AM, Daniel Shahaf wrote:
> >> I've made a quick sketch --- looks good?
> >
> > The suggestions look good.
> >
> > We could add some more tests, checking the behavior with SVN_NO_ERROR
> > input and one where all errors are tracing, which should return an error
> > from SVN_ERR_ASSERT().
> >
>
> I'm not sure about the 'assert' part (both due to the "testing that it
> has asserted" issue and due to the fact that it's mathematiclaly
> legitimaste to return an empty chain (aka SVN_NO_ERROR) in that case).
BTW, the malfunction API only promises an error in the category
SVN_ERR_MALFUNC_CATEGORY_START, so I have assumed we should have
a macro to decide whether an error code is in a given category:
#define SVN_ERROR_IN_CATEGORY(err, category) \
((category) == (((err) ? (err)->apr_err : 0) / SVN_ERR_CATEGORY_SIZE) * SVN_ERR_CATEGORY_SIZE)
{
if (SVN_ERROR_IN_CATEGORY(err, SVN_ERR_MALFUNC_CATEGORY_START)) { ... }
}
Re: svn_error_purge_tracing() and clearing the errors Re: svn
commit: r1051763 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_repos/commit.c
mod_dav_svn/version.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Thu, Dec 23, 2010 at 17:09:31 -0800:
> On 12/23/10 3:01 AM, Daniel Shahaf wrote:
>> I've made a quick sketch --- looks good?
>
> The suggestions look good.
>
> We could add some more tests, checking the behavior with SVN_NO_ERROR
> input and one where all errors are tracing, which should return an error
> from SVN_ERR_ASSERT().
>
I'm not sure about the 'assert' part (both due to the "testing that it
has asserted" issue and due to the fact that it's mathematiclaly
legitimaste to return an empty chain (aka SVN_NO_ERROR) in that case).
As to the rest, feel free to commit some tests (whether based on my
sketch or not); I'd rather not block you, and it might be a little while
before I can get to it myself...
> As an aside, there's a comment about adding an svn_boolean_t to
> svn_error_t instead of using strcmp(), which could be done in a totally
> separate commit.
Re: svn_error_purge_tracing() and clearing the errors Re: svn commit:
r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h
libsvn_repos/commit.c mod_dav_svn/version.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/23/10 3:01 AM, Daniel Shahaf wrote:
> I've made a quick sketch --- looks good?
The suggestions look good.
We could add some more tests, checking the behavior with SVN_NO_ERROR input and
one where all errors are tracing, which should return an error from
SVN_ERR_ASSERT().
As an aside, there's a comment about adding an svn_boolean_t to svn_error_t
instead of using strcmp(), which could be done in a totally separate commit.
>> Index: subversion/mod_dav_svn/deadprops.c
>> ===================================================================
>> --- subversion/mod_dav_svn/deadprops.c (revision 1052167)
>> +++ subversion/mod_dav_svn/deadprops.c (working copy)
>> @@ -222,18 +222,22 @@
>> + purged_serr->message = apr_xml_quote_string
>> + (purged_serr->pool,
>
> Our current policy is to have the '(' on the same line as the function name.
I looked through the hacking guide and didn't see anything, but doing some greps
through our code shows a disposition to this style:
$ cd subversion/
$ grep '^ *(' */*.c| wc -l
1757
$ grep '($' */*.c| wc -l
454
>> Index: subversion/mod_dav_svn/version.c
>> ===================================================================
>> --- subversion/mod_dav_svn/version.c (revision 1052167)
>> +++ subversion/mod_dav_svn/version.c (working copy)
>> @@ -922,16 +922,16 @@
>> {
>> if (serr)
>> {
>> - const char *post_commit_err;
>> - apr_err = serr->apr_err;
>> - post_commit_err = svn_repos__post_commit_error_str
>> - (serr, resource->pool);
>> - serr = SVN_NO_ERROR;
>> - ap_log_perror(APLOG_MARK, APLOG_ERR, apr_err, resource->pool,
>> + const char *post_commit_err = svn_repos__post_commit_error_str
>> + (serr, resource->pool);
>> + ap_log_perror(APLOG_MARK, APLOG_ERR, serr->apr_err,
>> + resource->pool,
>
> The rest looks good. (I am assuming --- didn't check --- that you
> updated all callers.)
Yes.
Blair
Re: svn_error_purge_tracing() and clearing the errors Re: svn
commit: r1051763 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_repos/commit.c
mod_dav_svn/version.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Wed, Dec 22, 2010 at 22:26:15 -0800:
> On 12/22/10 4:38 PM, Daniel Shahaf wrote:
>> 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:
>>>>> 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.
>
> I've attached a patch to review. It fixes both svn_error_purge_tracing()
> and the the behavior of svn_repos__post_commit_error_str().
>
Review below.
>>>>>> 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?
>
> Yes, I think it would. We should probably write a unit test for
> svn_error_purge_tracing().
>
> Blair
I've made a quick sketch --- looks good?
(I'll fix the "###" comment before committing.)
[[[
Index: subversion/tests/libsvn_subr/error-test.c
===================================================================
--- subversion/tests/libsvn_subr/error-test.c (revision 1052215)
+++ subversion/tests/libsvn_subr/error-test.c (working copy)
@@ -78,6 +78,34 @@ test_error_root_cause(apr_pool_t *pool)
return SVN_NO_ERROR;
}
+/* ### just make ../../libsvn_subr/error.c:is_tracing_link() semi-public */
+#ifdef SVN_ERR__TRACING
+#define is_tracing_link(err) \
+ ((err) && (err)->message && !strcmp((err)->message, SVN_ERR__TRACED))
+#else
+#define is_tracing_link(err) FALSE
+#endif
+
+static svn_error_t *
+test_error_purge_tracing(apr_pool_t *pool)
+{
+ svn_error_t *err, *err2, *child;
+
+ err = svn_error_quick_wrap(svn_error_create(SVN_ERR_BASE, NULL, "root error"),
+ "wrapped");
+ err = svn_error_quick_wrap(svn_error_create(SVN_ERR_BASE, err, "other error"),
+ "re-wrapped");
+
+ err2 = svn_error_purge_tracing(err);
+ for (child = err2; child; child = child->child)
+ if (is_tracing_link(child))
+ return svn_error_create(SVN_ERR_TEST_FAILED, err,
+ "Tracing link found after purging the following chain:");
+
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+}
+
/* The test table. */
@@ -86,5 +114,7 @@ struct svn_test_descriptor_t test_funcs[] =
SVN_TEST_NULL,
SVN_TEST_PASS2(test_error_root_cause,
"test svn_error_root_cause"),
+ SVN_TEST_PASS2(test_error_purge_tracing,
+ "test svn_error_purge_tracing"),
SVN_TEST_NULL
};
]]]
> Index: subversion/libsvn_subr/error.c
> ===================================================================
> --- subversion/libsvn_subr/error.c (revision 1052167)
> +++ subversion/libsvn_subr/error.c (working copy)
> @@ -349,39 +349,47 @@
> svn_error_purge_tracing(svn_error_t *err)
> {
> #ifdef SVN_ERR__TRACING
...
> return new_err;
> #else /* SVN_ERR__TRACING */
> return err;
+1
> Index: subversion/mod_dav_svn/deadprops.c
> ===================================================================
> --- subversion/mod_dav_svn/deadprops.c (revision 1052167)
> +++ subversion/mod_dav_svn/deadprops.c (working copy)
> @@ -222,18 +222,22 @@
> + purged_serr->message = apr_xml_quote_string
> + (purged_serr->pool,
Our current policy is to have the '(' on the same line as the function name.
> Index: subversion/mod_dav_svn/version.c
> ===================================================================
> --- subversion/mod_dav_svn/version.c (revision 1052167)
> +++ subversion/mod_dav_svn/version.c (working copy)
> @@ -922,16 +922,16 @@
> {
> if (serr)
> {
> - const char *post_commit_err;
> - apr_err = serr->apr_err;
> - post_commit_err = svn_repos__post_commit_error_str
> - (serr, resource->pool);
> - serr = SVN_NO_ERROR;
> - ap_log_perror(APLOG_MARK, APLOG_ERR, apr_err, resource->pool,
> + const char *post_commit_err = svn_repos__post_commit_error_str
> + (serr, resource->pool);
> + ap_log_perror(APLOG_MARK, APLOG_ERR, serr->apr_err,
> + resource->pool,
The rest looks good. (I am assuming --- didn't check --- that you
updated all callers.)
Re: svn_error_purge_tracing() and clearing the errors Re: svn
commit: r1051763 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_repos/commit.c
mod_dav_svn/version.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Wed, Dec 22, 2010 at 22:26:15 -0800:
> On 12/22/10 4:38 PM, Daniel Shahaf wrote:
>> 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:
>>>>> 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.
>
> I've attached a patch to review. It fixes both svn_error_purge_tracing()
> and the the behavior of svn_repos__post_commit_error_str().
>
Review below.
>>>>>> 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?
>
> Yes, I think it would. We should probably write a unit test for
> svn_error_purge_tracing().
>
> Blair
I've made a quick sketch --- looks good?
(I'll fix the "###" comment before committing.)
[[[
Index: subversion/tests/libsvn_subr/error-test.c
===================================================================
--- subversion/tests/libsvn_subr/error-test.c (revision 1052215)
+++ subversion/tests/libsvn_subr/error-test.c (working copy)
@@ -78,6 +78,34 @@ test_error_root_cause(apr_pool_t *pool)
return SVN_NO_ERROR;
}
+/* ### just make ../../libsvn_subr/error.c:is_tracing_link() semi-public */
+#ifdef SVN_ERR__TRACING
+#define is_tracing_link(err) \
+ ((err) && (err)->message && !strcmp((err)->message, SVN_ERR__TRACED))
+#else
+#define is_tracing_link(err) FALSE
+#endif
+
+static svn_error_t *
+test_error_purge_tracing(apr_pool_t *pool)
+{
+ svn_error_t *err, *err2, *child;
+
+ err = svn_error_quick_wrap(svn_error_create(SVN_ERR_BASE, NULL, "root error"),
+ "wrapped");
+ err = svn_error_quick_wrap(svn_error_create(SVN_ERR_BASE, err, "other error"),
+ "re-wrapped");
+
+ err2 = svn_error_purge_tracing(err);
+ for (child = err2; child; child = child->child)
+ if (is_tracing_link(child))
+ return svn_error_create(SVN_ERR_TEST_FAILED, err,
+ "Tracing link found after purging the following chain:");
+
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+}
+
/* The test table. */
@@ -86,5 +114,7 @@ struct svn_test_descriptor_t test_funcs[] =
SVN_TEST_NULL,
SVN_TEST_PASS2(test_error_root_cause,
"test svn_error_root_cause"),
+ SVN_TEST_PASS2(test_error_purge_tracing,
+ "test svn_error_purge_tracing"),
SVN_TEST_NULL
};
]]]
> Index: subversion/libsvn_subr/error.c
> ===================================================================
> --- subversion/libsvn_subr/error.c (revision 1052167)
> +++ subversion/libsvn_subr/error.c (working copy)
> @@ -349,39 +349,47 @@
> svn_error_purge_tracing(svn_error_t *err)
> {
> #ifdef SVN_ERR__TRACING
...
> return new_err;
> #else /* SVN_ERR__TRACING */
> return err;
+1
> Index: subversion/mod_dav_svn/deadprops.c
> ===================================================================
> --- subversion/mod_dav_svn/deadprops.c (revision 1052167)
> +++ subversion/mod_dav_svn/deadprops.c (working copy)
> @@ -222,18 +222,22 @@
> + purged_serr->message = apr_xml_quote_string
> + (purged_serr->pool,
Our current policy is to have the '(' on the same line as the function name.
> Index: subversion/mod_dav_svn/version.c
> ===================================================================
> --- subversion/mod_dav_svn/version.c (revision 1052167)
> +++ subversion/mod_dav_svn/version.c (working copy)
> @@ -922,16 +922,16 @@
> {
> if (serr)
> {
> - const char *post_commit_err;
> - apr_err = serr->apr_err;
> - post_commit_err = svn_repos__post_commit_error_str
> - (serr, resource->pool);
> - serr = SVN_NO_ERROR;
> - ap_log_perror(APLOG_MARK, APLOG_ERR, apr_err, resource->pool,
> + const char *post_commit_err = svn_repos__post_commit_error_str
> + (serr, resource->pool);
> + ap_log_perror(APLOG_MARK, APLOG_ERR, serr->apr_err,
> + resource->pool,
The rest looks good. (I am assuming --- didn't check --- that you
updated all callers.)
Re: svn_error_purge_tracing() and clearing the errors Re: svn commit:
r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h
libsvn_repos/commit.c mod_dav_svn/version.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/22/10 4:38 PM, Daniel Shahaf wrote:
> 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:
>> 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.
Yes.
>>>> 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.
I've attached a patch to review. It fixes both svn_error_purge_tracing() and
the the behavior of svn_repos__post_commit_error_str().
>>>>> 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?
Yes, I think it would. We should probably write a unit test for
svn_error_purge_tracing().
Blair
Re: svn_error_purge_tracing() and clearing the errors Re: svn
commit: r1051763 - in /subversion/trunk/subversion:
include/private/svn_repos_private.h libsvn_repos/commit.c
mod_dav_svn/version.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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:
>>>>>> blair@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
Re: svn_error_purge_tracing() and clearing the errors Re: svn commit:
r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h
libsvn_repos/commit.c mod_dav_svn/version.c
Posted by Blair Zajac <bl...@orcaware.com>.
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@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