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
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
- svn_error_t *tmp_err = err;
svn_error_t *new_err = NULL, *new_err_leaf = NULL;
if (! err)
return SVN_NO_ERROR;
- while (tmp_err)
+ do
{
/* Skip over any trace-only links. */
- while (tmp_err && is_tracing_link(tmp_err))
- tmp_err = tmp_err->child;
+ while (err && is_tracing_link(err))
+ err = err->child;
- /* Add a new link to the new chain (creating the chain if necessary). */
- if (! new_err)
+ if (err)
{
- new_err = tmp_err;
- new_err_leaf = new_err;
+ /* Copy the current error except for its child error pointer
+ into the new error. Share any message and source
+ filename strings from the error. */
+ svn_error_t *tmp_err = apr_palloc(err->pool, sizeof(*tmp_err));
+ *tmp_err = *err;
+ tmp_err->child = NULL;
+
+ /* Add a new link to the new chain (creating the chain if
+ necessary). */
+ if (! new_err)
+ {
+ new_err = tmp_err;
+ new_err_leaf = tmp_err;
+ }
+ else
+ {
+ new_err_leaf->child = tmp_err;
+ new_err_leaf = tmp_err;
+ }
+
+ /* Advance to the next link in the original chain. */
+ err = err->child;
}
- else
- {
- new_err_leaf->child = tmp_err;
- new_err_leaf = new_err_leaf->child;
- }
+ } while (err);
- /* Advance to the next link in the original chain. */
- tmp_err = tmp_err->child;
- }
-
/* If we get here, there had better be a real link in this error chain. */
SVN_ERR_ASSERT(new_err_leaf);
- /* Tie off the chain, and return its head. */
- new_err_leaf->child = NULL;
return new_err;
#else /* SVN_ERR__TRACING */
return err;
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 @@
resource->pool);
/* Prepare any hook failure message to get sent over the wire */
- serr = svn_error_purge_tracing(serr);
- if (serr && serr->apr_err == SVN_ERR_REPOS_HOOK_FAILURE)
- serr->message = apr_xml_quote_string(serr->pool, serr->message, 1);
-
- /* mod_dav doesn't handle the returned error very well, it
- generates its own generic error that will be returned to
- the client. Cache the detailed error here so that it can
- be returned a second time when the rollback mechanism
- triggers. */
if (serr)
- resource->info->revprop_error = svn_error_dup(serr);
+ {
+ svn_error_t *purged_serr = svn_error_purge_tracing(serr);
+ if (purged_serr->apr_err == SVN_ERR_REPOS_HOOK_FAILURE)
+ purged_serr->message = apr_xml_quote_string
+ (purged_serr->pool,
+ purged_serr->message, 1);
+ /* mod_dav doesn't handle the returned error very well, it
+ generates its own generic error that will be returned to
+ the client. Cache the detailed error here so that it can
+ be returned a second time when the rollback mechanism
+ triggers. */
+ resource->info->revprop_error = svn_error_dup(purged_serr);
+ }
+
/* Tell the logging subsystem about the revprop change. */
dav_svn__operational_log(resource->info,
svn_log__change_rev_prop(
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,
"commit of r%ld succeeded, but an error occurred "
"after the commit: '%s'",
new_rev,
post_commit_err);
+ svn_error_clear(serr);
+ serr = SVN_NO_ERROR;
}
}
else
@@ -1421,6 +1421,7 @@
### client some other way than hijacking the post-commit
### error message.*/
post_commit_err = svn_repos__post_commit_error_str(serr, pool);
+ svn_error_clear(serr);
serr = SVN_NO_ERROR;
}
}
Index: subversion/mod_dav_svn/util.c
===================================================================
--- subversion/mod_dav_svn/util.c (revision 1052167)
+++ subversion/mod_dav_svn/util.c (working copy)
@@ -111,7 +111,7 @@
/* Remove the trace-only error chain links. We need predictable
protocol behavior regardless of whether or not we're in a
debugging build. */
- serr = svn_error_purge_tracing(serr);
+ svn_error_t *purged_serr = svn_error_purge_tracing(serr);
/* ### someday mod_dav_svn will send back 'rich' error tags, much
finer grained than plain old svn_error_t's. But for now, all
@@ -122,7 +122,7 @@
appropriate HTTP status code. If no more appropriate HTTP
status code maps to the Subversion error code, use the one
suggested status provided by the caller. */
- switch (serr->apr_err)
+ switch (purged_serr->apr_err)
{
case SVN_ERR_FS_NOT_FOUND:
status = HTTP_NOT_FOUND;
@@ -139,11 +139,12 @@
/* add other mappings here */
}
- derr = build_error_chain(pool, serr, status);
+ derr = build_error_chain(pool, purged_serr, status);
if (message != NULL
- && serr->apr_err != SVN_ERR_REPOS_HOOK_FAILURE)
+ && purged_serr->apr_err != SVN_ERR_REPOS_HOOK_FAILURE)
/* Don't hide hook failures; we might hide the error text */
- derr = dav_push_error(pool, status, serr->apr_err, message, derr);
+ derr = dav_push_error(pool, status, purged_serr->apr_err,
+ message, derr);
/* Now, destroy the Subversion error. */
svn_error_clear(serr);
Index: subversion/include/svn_error.h
===================================================================
--- subversion/include/svn_error.h (revision 1052167)
+++ subversion/include/svn_error.h (working copy)
@@ -315,12 +315,19 @@
#endif
/**
- * Purge from @a err and its child chain any links associated with
- * error tracing placeholders, and return the new top-level error
- * chain item. @a err should be considered unusable after passing
- * through this function, but should *not* be cleared (as the returned
- * error shares memory with @a err). @a err can be #SVN_NO_ERROR.
+ * Returns an error chain that is based on @a err's error chain but
+ * does not include any error tracing placeholders. @a err is not
+ * modified, except for any allocations using its pool.
*
+ * The returned error chain is allocated from @a err's pool and shares
+ * its message and source filename character arrays. The returned
+ * error chain should *not* be cleared because it is not a fully
+ * fledged error chain, only clearing @a err should be done to clear
+ * the returned error chain. If @a err is cleared, then the returned
+ * error chain is unusable.
+ *
+ * @a err can be #SVN_NO_ERROR.
+ *
* @since New in 1.7.
*/
svn_error_t *svn_error_purge_tracing(svn_error_t *err);
Index: subversion/include/private/svn_repos_private.h
===================================================================
--- subversion/include/private/svn_repos_private.h (revision 1052167)
+++ subversion/include/private/svn_repos_private.h (working copy)
@@ -91,8 +91,7 @@
* the post-commit hook. Any error tracing placeholders in the error
* chain are skipped over.
*
- * This function clears @a err and it should not be used after passing
- * it to this function.
+ * This function does not modify @a err.
*
* ### This method should not be necessary, but there are a few
* ### places, e.g. mod_dav_svn, where only a single error message
Index: subversion/libsvn_repos/commit.c
===================================================================
--- subversion/libsvn_repos/commit.c (revision 1052167)
+++ subversion/libsvn_repos/commit.c (working copy)
@@ -705,11 +705,6 @@
: _("(no error message)"));
}
- /* Because svn_error_purge_tracing() was used on the input error,
- the purged error must either be cleared here or returned to the
- caller. This function just clears it. */
- svn_error_clear(err);
-
return msg;
}
@@ -743,6 +738,7 @@
(to be reported back to the client, who will probably
display it as a warning) and clear the error. */
post_commit_err = svn_repos__post_commit_error_str(err, pool);
+ svn_error_clear(err);
err = SVN_NO_ERROR;
}
}
