[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

[PATCH] Re: Error exit conditions from svn binaries

From: Daniel Shahaf <d.s_at_daniel.shahaf.co.il>
Date: Tue, 22 Apr 2008 17:37:09 +0300 (IDT)

Daniel Shahaf wrote on Tue, 22 Apr 2008 at 00:36 +0300:
> Stefan Sperling noted on IRC that the svn command-line tools respond to
> errors in main() in different ways:
>
> 1. svn_handle_error2(force = False);
> 2. svn_handle_error2(force = True);
> 3. return svn_cmdline_handle_exit_error(svn_error_t *error);
> 4. SVN_INT_ERR(expr);
>
> which, respectively,
>
> 1. print an error chain and return to caller;
> (this call is sometimes used to print warnings)
> 2. print an error chain and abort();
> 3. Call svn_handle_error2(@a error, stderr, FALSE, @a prefix), clear
> @a error, destroy @a pool iff it is non-NULL, and return
> EXIT_FAILURE;
> 4. Evaluate @a expr. If it yields an error, handle that error and
> return @c EXIT_FAILURE.
>
> svn_handle_error2 has the following comment:
>
> if (fatal)
> /* XXX Shouldn't we exit(1) here instead, so that atexit handlers
> get called? --xbc */
> abort();
>
> Further, in r24415 Malcolm Rowe converted numerous exit paths to use
> svn_cmdline_handle_exit_error(), noting that that fixed error leaks and
> prevented aborts in maintainer mode. (If I understand correctly, the
> aborts were caused by exiting the process before destroying the
> top-level pool.)
>

I did not understand correctly; the aborts are caused by leaking errors,
and are not caused by not destroying the global pool. (The log message
says that explicitly; I must have read around it.)

Is there any objection to changing the abort() in svn_handle_error2() to
exit()?

Daniel

[[[
Do not abort in svn_handle_error2().

* subversion/include/svn_error.h
  (svn_handle_error2):
    Document that the error is cleared before exiting.

* subversion/libsvn_subr/error.c
  (svn_handle_error2):
    When fatal == TRUE, clear the error and call exit() instead of abort().

Patch by: Daniel Shahaf <d.s_at_daniel.shahaf.co.il>
Suggested by: stsp
]]]

Index: subversion/include/svn_error.h
===================================================================
--- subversion/include/svn_error.h (revision 30750)
+++ subversion/include/svn_error.h (working copy)
@@ -176,9 +176,9 @@ void svn_error_clear(svn_error_t *error);
 
 /**
  * Very basic default error handler: print out error stack @a error to the
- * stdio stream @a stream, with each error prefixed by @a prefix, and quit
- * iff the @a fatal flag is set. Allocations are performed in the @a error's
- * pool.
+ * stdio stream @a stream, with each error prefixed by @a prefix; quit and
+ * clear @a error iff the @a fatal flag is set. Allocations are performed
+ * in the @a error's pool.
  *
  * If you're not sure what prefix to pass, just pass "svn: ". That's
  * what code that used to call svn_handle_error() and now calls
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 30750)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -375,6 +375,7 @@ svn_handle_error2(svn_error_t *err,
      use a subpool. */
   apr_pool_t *subpool;
   apr_array_header_t *empties;
+ svn_error_t *tmp_err;
 
   /* ### The rest of this file carefully avoids using svn_pool_*(),
      preferring apr_pool_*() instead. I can't remember why -- it may
@@ -383,16 +384,17 @@ svn_handle_error2(svn_error_t *err,
   apr_pool_create(&subpool, err->pool);
   empties = apr_array_make(subpool, 0, sizeof(apr_status_t));
 
- while (err)
+ tmp_err = err;
+ while (tmp_err)
     {
       int i;
       svn_boolean_t printed_already = FALSE;
 
- if (! err->message)
+ if (! tmp_err->message)
         {
           for (i = 0; i < empties->nelts; i++)
             {
- if (err->apr_err == APR_ARRAY_IDX(empties, i, apr_status_t) )
+ if (tmp_err->apr_err == APR_ARRAY_IDX(empties, i, apr_status_t) )
                 {
                   printed_already = TRUE;
                   break;
@@ -402,23 +404,28 @@ svn_handle_error2(svn_error_t *err,
 
       if (! printed_already)
         {
- print_error(err, stream, prefix);
- if (! err->message)
+ print_error(tmp_err, stream, prefix);
+ if (! tmp_err->message)
             {
- APR_ARRAY_PUSH(empties, apr_status_t) = err->apr_err;
+ APR_ARRAY_PUSH(empties, apr_status_t) = tmp_err->apr_err;
             }
         }
 
- err = err->child;
+ tmp_err = tmp_err->child;
     }
 
   svn_pool_destroy(subpool);
 
   fflush(stream);
   if (fatal)
- /* XXX Shouldn't we exit(1) here instead, so that atexit handlers
- get called? --xbc */
- abort();
+ {
+ /* Avoid abort()s in maintainer mode. */
+ svn_error_clear(err);
+
+ /* We exit(1) here instead of abort()ing so that atexit handlers
+ get called. */
+ exit(EXIT_FAILURE);
+ }
 }
 
 

> Given that aborts are bad, would it make sense to (a) implement xbc's
> comment quoted above; (b) change all exit paths of types (2) and (4) to
> use svn_cmdline_handle_exit_error, and document that for posterity?
>
> Daniel
>
>
> P.S. Stefan also suggested adding a warning to SVN_INT_ERR that it
> should not be used in a main() due to maintainer-mode aborts;
> I suggested a wording here: <http://paste.lisp.org/display/59488>.
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-22 16:45:07 CEST

This is an archived mail posted to the Subversion Dev mailing list.