You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Eric Gillespie <ep...@pretzelnet.org> on 2008/04/04 02:06:56 UTC
Interaction between svn_error_t and bindings exceptions
Release note: this has been broken for long enough, it need not
hold up 1.5.
Background
==========
In Python (and Ruby and Perl (and ...?)) we have exceptions. In
Subversion we have svn_error_t. When Subversion functions return
an error object, we convert that into the exception system. That
stuff is simple and we need not worry about it.
The tricky bit is callbacks. This is Python calling swig-py
calling Subversion calling swig-py calling Python:
reporter.finish_report (Python)
-> _wrap_svn_ra_reporter3_invoke_finish_report (swig-py)
-> finish_report (Subversion repos/reporter.c)
-> sw_apply_textdelta (swig-py swigutil_py.c)
-> py_apply_textdelta (Python)
(If you're trying to follow along looking at the source files,
note that both sw_apply_textdelta and py_apply_textdelta are both
called 'apply_textdelta'; I mangle them here for clarity.)
What happens when a Python callback raises an exception?
- sw_apply_textdelta (and all the other callback wrappers) notices
the exception and creates and returns a new svn_error_t with
apr_err set to SVN_ERR_SWIG_PY_EXCEPTION_SET.
- finish_report passes it along via SVN_ERR()
- _wrap_svn_ra_reporter3_invoke_finish_report (and all other
Subversion wrappers) check the svn_error_t return. If
SVN_NO_ERROR, no problem; if not SVN_ERR_SWIG_PY_EXCEPTION_SET,
create an exception and raise that (see first Background paragraph).
But, if SVN_ERR_SWIG_PY_EXCEPTION_SET, clear the error and
return NULL, knowing that Python has py_apply_textdelta's exception.
- reporter.finish_report: we're back in Python, and the exception
propagages up the stack until a matching except block
Parallel errors
===============
That's all well and good until we consider the problem of
handling exceptions while an exception is already in progress.
Consider two examples, first pure Python, then pure C:
Pure Python:
def Cleanup():
raise CleanupError
def DoSomething():
try:
...
except:
exc_type, exc_val, exc_tb = sys.exc_info()
try:
Cleanup()
except:
pass
raise exc_type, exc_val, exc_tb
Pure C:
Cleanup() {
return svn_error_t("abort error");
}
DoSomething() {
svn_error_t *err = ...;
if (err != SVN_NO_ERROR)
svn_error_clear(Cleanup());
return err;
}
Simple enough. See the end of repos/reporter.c:finish_report for
this pattern in action.
The problem
===========
But what happens if DoSomething is C and Cleanup is Python?
Don't cross the streams! Replace DoSomething and Cleanup with
finish_report and our Python abort_edit from the Background
example.
finish_report clears the SVN_ERR_SWIG_PY_EXCEPTION_SET
svn_error_t but it doesn't touch the Python interpreter's
exception state! Our exception/svn_error_t mapping is
asymmetrical; we map the Python exception into an svn_error_t,
but we don't map the clearing of an svn_error_t back to a
clearing of the Python exception state:
Pure Subversion:
my_apply_textdelta()
{
return svn_error_t("delta error");
}
my_abort_edit()
{
return svn_error_t("abort error");
}
my_diff() {
...
err = reporter->finish_report();
// err is the the "delta error", not "abort error"
// the reporter discarded the "abort error"
}
Python calling swig-py calling Subversion calling swig-py calling Python:
def my_apply_textdelta():
raise Exception("delta error")
def my_abort_edit():
raise Exception("abort error")
def my_diff():
...
try:
reporter.finish_report()
except Exception, err:
# err is the the "abort error", not "delta error"
# the reporter discarded the SVN_ERR_SWIG_PY_EXCEPTION_SET svn_error_t
# but DID NOT clear the Python exception!
Solutions
=========
Somehow the svn_error_clear at the end of reporter.c:
finish_report has to trigger clearing the Python exception.
All solutions involve modifying at least
svn_types.swg:%typemap(out) svn_error_t * and
swigutil_py.c:callback_exception_error .
- Extend svn_error_t with bindings callback/baton
svn_error_t constructors set this to NULL, and bindings may
then fill it in:
static void *callback_exception_error_cb(void *baton) {
PyObject (*type, *value, *traceback) = baton;
Py_XDECREF them all
}
static svn_error_t *callback_exception_error(void)
{
PyObject *type, *value, *traceback;
svn_error_t *err = svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
"Python callback raised an exception");
PyErr_Fetch(&type, &value, &traceback);
err->ffi_func = callback_exception_error_cb;
err->ffi_baton = Tuple(type, value, traceback);
}
%typemap(out) svn_error_t * {
if ($1 != NULL) {
if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
(type, value, traceback) = $1->ffi_baton;
$1->ffi_func = NULL;
if (type != NULL)
PyErr_Restore(type, value, traceback);
svn_error_clear($1);
} else
svn_swig_py_svn_exception($1);
SWIG_fail;
}
Py_INCREF(Py_None);
$result = Py_None;
}
void
svn_error_clear(svn_error_t *err) {
if (err->ffi_callback != NULL)
err->ffi_callback(err->ffi_baton);
...
}
In theory, we can extend even non-opaque structures at the end
as long as we require that callers always use a constructor.
This seems to be the case for svn_error_t, so revving would not
be necessary. Glasser and kfogel seem fine with this, but epg
is quite wary.
- Variant: rev svn_error_t
Dodges potential (or imaginary) problems with extending an
existing structure, but gives everyone involved a heart attack.
If there's any type we never want to rev, this is the one...
- Provide an svn_error_t lookalike in swig-py
struct swig_py_svn_error_t {
struct svn_error_t err;
PyObject *type, *value, *traceback;
}
static void *callback_exception_error_cb(void *baton) {
PyObject (*type, *value, *traceback) = baton;
Py_XDECREF them all
}
static svn_error_t *callback_exception_error(void)
{
PyObject *type, *value, *traceback;
svn_error_t *err = svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
"Python callback raised an exception");
swig_py_svn_error_t *perr = swig_py_svn_error_t();
copy err fields over to perr;
PyErr_Fetch(&perr->type, &perr->value, &perr->traceback);
register a cleanup handler for callback_exception_error_cb on the err pool;
}
%typemap(out) svn_error_t * {
if ($1 != NULL) {
if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
unregister pool cleanup handler;
if ((py_err)$1->type != NULL)
PyErr_Restore(py_err)$1->type, py_err)$1->value,
py_err)$1->traceback);
svn_error_clear($1);
} else
svn_swig_py_svn_exception($1);
SWIG_fail;
}
Py_INCREF(Py_None);
$result = Py_None;
}
- Global dict mapping svn_error_t to exception state
static svn_error_t *callback_exception_error(void)
{
PyObject *type, *value, *traceback;
svn_error_t *err = svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
"Python callback raised an exception");
PyErr_Fetch(&type, &value, &traceback);
Global_Err_Map[err] = (type, value, traceback)
}
%typemap(out) svn_error_t * {
if ($1 != NULL) {
if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
(type, value, traceback) = Global_Err_Map[$1]
del Global_Err_Map[$1]
if (type != NULL)
PyErr_Restore(type, value, traceback);
svn_error_clear($1);
} else
svn_swig_py_svn_exception($1);
SWIG_fail;
}
Py_INCREF(Py_None);
$result = Py_None;
}
I'm not sure how to create the global dict thread-safely.
Worse, the exception state is leaked when one of the error from
abort_edit is discarded.
- Use pool cleanup like the lookalike solution, but with userdata
This is my favorite solution, but I didn't think of it until
after Glasser left, so it hasn't had the obvious reasons why it
couldn't possibly work pointed out. Glasser? ;->
If we're using the pool cleanup handler to free the exception
objects, why not also store them as userdata on that field?
Then we don't need to extend or rev any types, fake out any
types, or manage a global dict (with leaks).
(Side note: swigutil_py.c:callback_bad_return_error is broken,
and the TypeError gets clobbered by a SubversionError for the
APR_EGENERAL it returns; instead it needs to return
callback_exception_error()).
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Interaction between svn_error_t and bindings exceptions
Posted by David James <ja...@cs.toronto.edu>.
On Fri, Apr 4, 2008 at 10:48 AM, David Glasser <gl...@davidglasser.net> wrote:
>
> On Thu, Apr 3, 2008 at 9:53 PM, David Glasser <gl...@davidglasser.net> wrote:
> >
> > On Thu, Apr 3, 2008 at 7:06 PM, Eric Gillespie <ep...@pretzelnet.org> wrote:
> > > - Provide an svn_error_t lookalike in swig-py
> > >
> > > struct swig_py_svn_error_t {
> > > struct svn_error_t err;
> > > PyObject *type, *value, *traceback;
> > > }
> > > static void *callback_exception_error_cb(void *baton) {
> > > PyObject (*type, *value, *traceback) = baton;
> > > Py_XDECREF them all
> > > }
> > > static svn_error_t *callback_exception_error(void)
> > > {
> > > PyObject *type, *value, *traceback;
> > > svn_error_t *err = svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
> > > "Python callback raised an exception");
> > > swig_py_svn_error_t *perr = swig_py_svn_error_t();
> > > copy err fields over to perr;
> > > PyErr_Fetch(&perr->type, &perr->value, &perr->traceback);
> > > register a cleanup handler for callback_exception_error_cb on the err pool;
> > > }
> > >
> > > %typemap(out) svn_error_t * {
> > > if ($1 != NULL) {
> > > if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
> > > unregister pool cleanup handler;
> > > if ((py_err)$1->type != NULL)
> > > PyErr_Restore(py_err)$1->type, py_err)$1->value,
> > > py_err)$1->traceback);
> > > svn_error_clear($1);
> > > } else
> > > svn_swig_py_svn_exception($1);
> > > SWIG_fail;
> > > }
> > > Py_INCREF(Py_None);
> > > $result = Py_None;
> > > }
> >
> > I almost completely like this one, except for worries about handling
> > crap like svn_error_dup and svn_error_compose, which are going to do a
> > poor job of copying extra data they don't know about.
> >
> >
> > > - Use pool cleanup like the lookalike solution, but with userdata
> > >
> > > This is my favorite solution, but I didn't think of it until
> > > after Glasser left, so it hasn't had the obvious reasons why it
> > > couldn't possibly work pointed out. Glasser? ;->
> > >
> > > If we're using the pool cleanup handler to free the exception
> > > objects, why not also store them as userdata on that field?
> > > Then we don't need to extend or rev any types, fake out any
> > > types, or manage a global dict (with leaks).
> > >
> > > (Side note: swigutil_py.c:callback_bad_return_error is broken,
> > > and the TypeError gets clobbered by a SubversionError for the
> > > APR_EGENERAL it returns; instead it needs to return
> > > callback_exception_error()).
> >
> > This also doesn't work very well with chained errors, I think, since
> > they share a pool (not to mention dup/compose).
> >
> >
> >
> > ... OK. So this is a bit of a hack, but it would work. We can do the
> > "bigger punned struct" thing, but keep it internal to
> > libsvn_subr/error.c.
> >
> > Define an error category SVN_ERR_WRAPPED_* in svn_error_codes.h.
> >
> > In error.c, do
> >
> > struct wrapped_error {
> >
> > struct svn_error_t err;
> > void *baton;
> > };
> >
> > #define ERR_IS_WRAPPED(x) (((x) >= SVN_ERR_WRAPPED_CATEGORY_START) &&
> > ((x) < SVN_ERR_CATEGORY_AFTER_WRAPPED_START))
> > #define ERR_STRUCT_SIZE(x) (ERR_IS_WRAPPED(x) ? sizeof(struct
> > wrapped_error) : sizeof(struct svn_error_t))
> >
> > Everywhere in error.c that allocates an error (make_error_internal,
> > svn_error_compose, svn_error_dup) would use ERR_STRUCT_SIZE rather
> > than a hardcoded sizeof. The "*a = *b" lines in svn_error_compose and
> > svn_error_dup might need to be tweaked a bit as well.
> >
> > Finally, just add
> >
> > void *svn_error_get_wrap_baton(svn_error_t *err)
> > {
> > struct wrapped_error *w = err;
> > assert(ERR_IS_WRAPPED(err->apr_err));
> > return w->baton;
> > }
> >
> > void svn_error_set_wrap_baton(svn_error_t *err, void *baton)
> > {
> > struct wrapped_error *w = err;
> > assert(ERR_IS_WRAPPED(err->apr_err));
> > w->baton = baton;
> > }
> >
> > No memory leaks. No implementation details leaked outside error.c
> > (and documentation of some binding-related APIs). Allows you to
> > define SVN_ERR_WRAPPED_PYTHON and SVN_ERR_WRAPPED_PERL with each
> > language only unwrapping its own exceptions (and leaving the other
> > language's as opaque). Exceptions and errors never get out of touch
> > with each other.
> >
> > Finally, document explicitly that err->pool is cleared/destroyed if
> > and only if svn_error_clear or svn_handle_errorN is called, and set a
> > decrefing pool handler in Python. (For completeness, perhaps wrapped
> > errors also need callbacks that are called when svn_error_dup/compose
> > are called, since swigpy will need to tweak some refcounts there as
> > well. And while you're doing that, eh, maybe do the clear thing as a
> > callback rather than as a pool cleanup handler.
> >
> > Does this sound non-insane?
>
> A slight refinement:
>
> * Ban creating SVN_ERR_WRAPPED_* errors with the normal functions
> (svn_error_create[f], svn_error_wrap_apr).
>
> * Actually expose svn_error_bindings_wrapper_t.
>
> * Make a new svn_error_wrap_from_bindings:
>
> typedef void (svn_error_clear_callback)(svn_error_bindings_wrapper_t *err);
> typedef void (svn_error_dup_callback)(svn_error_bindings_wrapper_ *err);
>
> svn_error_t *svn_error_wrap_external_error(apr_status_t apr_err, void
> *baton, svn_error_clear_callback *clear_cb, svn_error_dup_callback
> *dup_cb)
> {
> assert(ERR_IS_WRAPPED(apr_err));
> [ make the error, with the bigger size, as above ]
> store baton, clear_cb, dup_cb in the "extra" error fields
> }
>
> The clear callback is called from svn_error_clear and
> svn_handle_error; the dup callback is called from svn_error_dup;
> svn_error_compose doesn't need to do anything. No pool callbacks,
> either.
>
> For Python, the "clear" callback does a decref and the "dup" callback
> does an incref. The "return-error-from-C-to-Python" code checks to
> see if it's SVN_ERR_WRAPPED_SWIG_PYTHON, and in that case casts to
> svn_error_bindings_wrapper_t * and pulls out the baton.
This sounds great! I like that you managed to keep all of the changes
contained so that they won't affect code other than the bindings.
The ctypes bindings have dodged this issue so far because we don't
allow Python exceptions to be thrown from inside callbacks. Instead we
require (right now) that the Python callbacks return appropriate error
codes. This also therefore dodges the requirement that the exceptions
propagate up the stack.
When we do extend the higher level ctypes Python bindings to support
catching and wrapping exceptions inside callbacks, we'll benefit from
your work here.
Cheers,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Interaction between svn_error_t and bindings exceptions
Posted by David Glasser <gl...@davidglasser.net>.
On Thu, Apr 3, 2008 at 9:53 PM, David Glasser <gl...@davidglasser.net> wrote:
>
> On Thu, Apr 3, 2008 at 7:06 PM, Eric Gillespie <ep...@pretzelnet.org> wrote:
> > - Provide an svn_error_t lookalike in swig-py
> >
> > struct swig_py_svn_error_t {
> > struct svn_error_t err;
> > PyObject *type, *value, *traceback;
> > }
> > static void *callback_exception_error_cb(void *baton) {
> > PyObject (*type, *value, *traceback) = baton;
> > Py_XDECREF them all
> > }
> > static svn_error_t *callback_exception_error(void)
> > {
> > PyObject *type, *value, *traceback;
> > svn_error_t *err = svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
> > "Python callback raised an exception");
> > swig_py_svn_error_t *perr = swig_py_svn_error_t();
> > copy err fields over to perr;
> > PyErr_Fetch(&perr->type, &perr->value, &perr->traceback);
> > register a cleanup handler for callback_exception_error_cb on the err pool;
> > }
> >
> > %typemap(out) svn_error_t * {
> > if ($1 != NULL) {
> > if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
> > unregister pool cleanup handler;
> > if ((py_err)$1->type != NULL)
> > PyErr_Restore(py_err)$1->type, py_err)$1->value,
> > py_err)$1->traceback);
> > svn_error_clear($1);
> > } else
> > svn_swig_py_svn_exception($1);
> > SWIG_fail;
> > }
> > Py_INCREF(Py_None);
> > $result = Py_None;
> > }
>
> I almost completely like this one, except for worries about handling
> crap like svn_error_dup and svn_error_compose, which are going to do a
> poor job of copying extra data they don't know about.
>
>
> > - Use pool cleanup like the lookalike solution, but with userdata
> >
> > This is my favorite solution, but I didn't think of it until
> > after Glasser left, so it hasn't had the obvious reasons why it
> > couldn't possibly work pointed out. Glasser? ;->
> >
> > If we're using the pool cleanup handler to free the exception
> > objects, why not also store them as userdata on that field?
> > Then we don't need to extend or rev any types, fake out any
> > types, or manage a global dict (with leaks).
> >
> > (Side note: swigutil_py.c:callback_bad_return_error is broken,
> > and the TypeError gets clobbered by a SubversionError for the
> > APR_EGENERAL it returns; instead it needs to return
> > callback_exception_error()).
>
> This also doesn't work very well with chained errors, I think, since
> they share a pool (not to mention dup/compose).
>
>
>
> ... OK. So this is a bit of a hack, but it would work. We can do the
> "bigger punned struct" thing, but keep it internal to
> libsvn_subr/error.c.
>
> Define an error category SVN_ERR_WRAPPED_* in svn_error_codes.h.
>
> In error.c, do
>
> struct wrapped_error {
>
> struct svn_error_t err;
> void *baton;
> };
>
> #define ERR_IS_WRAPPED(x) (((x) >= SVN_ERR_WRAPPED_CATEGORY_START) &&
> ((x) < SVN_ERR_CATEGORY_AFTER_WRAPPED_START))
> #define ERR_STRUCT_SIZE(x) (ERR_IS_WRAPPED(x) ? sizeof(struct
> wrapped_error) : sizeof(struct svn_error_t))
>
> Everywhere in error.c that allocates an error (make_error_internal,
> svn_error_compose, svn_error_dup) would use ERR_STRUCT_SIZE rather
> than a hardcoded sizeof. The "*a = *b" lines in svn_error_compose and
> svn_error_dup might need to be tweaked a bit as well.
>
> Finally, just add
>
> void *svn_error_get_wrap_baton(svn_error_t *err)
> {
> struct wrapped_error *w = err;
> assert(ERR_IS_WRAPPED(err->apr_err));
> return w->baton;
> }
>
> void svn_error_set_wrap_baton(svn_error_t *err, void *baton)
> {
> struct wrapped_error *w = err;
> assert(ERR_IS_WRAPPED(err->apr_err));
> w->baton = baton;
> }
>
> No memory leaks. No implementation details leaked outside error.c
> (and documentation of some binding-related APIs). Allows you to
> define SVN_ERR_WRAPPED_PYTHON and SVN_ERR_WRAPPED_PERL with each
> language only unwrapping its own exceptions (and leaving the other
> language's as opaque). Exceptions and errors never get out of touch
> with each other.
>
> Finally, document explicitly that err->pool is cleared/destroyed if
> and only if svn_error_clear or svn_handle_errorN is called, and set a
> decrefing pool handler in Python. (For completeness, perhaps wrapped
> errors also need callbacks that are called when svn_error_dup/compose
> are called, since swigpy will need to tweak some refcounts there as
> well. And while you're doing that, eh, maybe do the clear thing as a
> callback rather than as a pool cleanup handler.
>
> Does this sound non-insane?
A slight refinement:
* Ban creating SVN_ERR_WRAPPED_* errors with the normal functions
(svn_error_create[f], svn_error_wrap_apr).
* Actually expose svn_error_bindings_wrapper_t.
* Make a new svn_error_wrap_from_bindings:
typedef void (svn_error_clear_callback)(svn_error_bindings_wrapper_t *err);
typedef void (svn_error_dup_callback)(svn_error_bindings_wrapper_ *err);
svn_error_t *svn_error_wrap_external_error(apr_status_t apr_err, void
*baton, svn_error_clear_callback *clear_cb, svn_error_dup_callback
*dup_cb)
{
assert(ERR_IS_WRAPPED(apr_err));
[ make the error, with the bigger size, as above ]
store baton, clear_cb, dup_cb in the "extra" error fields
}
The clear callback is called from svn_error_clear and
svn_handle_error; the dup callback is called from svn_error_dup;
svn_error_compose doesn't need to do anything. No pool callbacks,
either.
For Python, the "clear" callback does a decref and the "dup" callback
does an incref. The "return-error-from-C-to-Python" code checks to
see if it's SVN_ERR_WRAPPED_SWIG_PYTHON, and in that case casts to
svn_error_bindings_wrapper_t * and pulls out the baton.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Interaction between svn_error_t and bindings exceptions
Posted by David Glasser <gl...@davidglasser.net>.
On Thu, Apr 3, 2008 at 7:06 PM, Eric Gillespie <ep...@pretzelnet.org> wrote:
> - Provide an svn_error_t lookalike in swig-py
>
> struct swig_py_svn_error_t {
> struct svn_error_t err;
> PyObject *type, *value, *traceback;
> }
> static void *callback_exception_error_cb(void *baton) {
> PyObject (*type, *value, *traceback) = baton;
> Py_XDECREF them all
> }
> static svn_error_t *callback_exception_error(void)
> {
> PyObject *type, *value, *traceback;
> svn_error_t *err = svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
> "Python callback raised an exception");
> swig_py_svn_error_t *perr = swig_py_svn_error_t();
> copy err fields over to perr;
> PyErr_Fetch(&perr->type, &perr->value, &perr->traceback);
> register a cleanup handler for callback_exception_error_cb on the err pool;
> }
>
> %typemap(out) svn_error_t * {
> if ($1 != NULL) {
> if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
> unregister pool cleanup handler;
> if ((py_err)$1->type != NULL)
> PyErr_Restore(py_err)$1->type, py_err)$1->value,
> py_err)$1->traceback);
> svn_error_clear($1);
> } else
> svn_swig_py_svn_exception($1);
> SWIG_fail;
> }
> Py_INCREF(Py_None);
> $result = Py_None;
> }
I almost completely like this one, except for worries about handling
crap like svn_error_dup and svn_error_compose, which are going to do a
poor job of copying extra data they don't know about.
> - Use pool cleanup like the lookalike solution, but with userdata
>
> This is my favorite solution, but I didn't think of it until
> after Glasser left, so it hasn't had the obvious reasons why it
> couldn't possibly work pointed out. Glasser? ;->
>
> If we're using the pool cleanup handler to free the exception
> objects, why not also store them as userdata on that field?
> Then we don't need to extend or rev any types, fake out any
> types, or manage a global dict (with leaks).
>
> (Side note: swigutil_py.c:callback_bad_return_error is broken,
> and the TypeError gets clobbered by a SubversionError for the
> APR_EGENERAL it returns; instead it needs to return
> callback_exception_error()).
This also doesn't work very well with chained errors, I think, since
they share a pool (not to mention dup/compose).
... OK. So this is a bit of a hack, but it would work. We can do the
"bigger punned struct" thing, but keep it internal to
libsvn_subr/error.c.
Define an error category SVN_ERR_WRAPPED_* in svn_error_codes.h.
In error.c, do
struct wrapped_error {
struct svn_error_t err;
void *baton;
};
#define ERR_IS_WRAPPED(x) (((x) >= SVN_ERR_WRAPPED_CATEGORY_START) &&
((x) < SVN_ERR_CATEGORY_AFTER_WRAPPED_START))
#define ERR_STRUCT_SIZE(x) (ERR_IS_WRAPPED(x) ? sizeof(struct
wrapped_error) : sizeof(struct svn_error_t))
Everywhere in error.c that allocates an error (make_error_internal,
svn_error_compose, svn_error_dup) would use ERR_STRUCT_SIZE rather
than a hardcoded sizeof. The "*a = *b" lines in svn_error_compose and
svn_error_dup might need to be tweaked a bit as well.
Finally, just add
void *svn_error_get_wrap_baton(svn_error_t *err)
{
struct wrapped_error *w = err;
assert(ERR_IS_WRAPPED(err->apr_err));
return w->baton;
}
void svn_error_set_wrap_baton(svn_error_t *err, void *baton)
{
struct wrapped_error *w = err;
assert(ERR_IS_WRAPPED(err->apr_err));
w->baton = baton;
}
No memory leaks. No implementation details leaked outside error.c
(and documentation of some binding-related APIs). Allows you to
define SVN_ERR_WRAPPED_PYTHON and SVN_ERR_WRAPPED_PERL with each
language only unwrapping its own exceptions (and leaving the other
language's as opaque). Exceptions and errors never get out of touch
with each other.
Finally, document explicitly that err->pool is cleared/destroyed if
and only if svn_error_clear or svn_handle_errorN is called, and set a
decrefing pool handler in Python. (For completeness, perhaps wrapped
errors also need callbacks that are called when svn_error_dup/compose
are called, since swigpy will need to tweak some refcounts there as
well. And while you're doing that, eh, maybe do the clear thing as a
callback rather than as a pool cleanup handler.
Does this sound non-insane?
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org