Re: [PATCH] Fill in callbacks for svn_ra_callbacks2_t()

On 5/25/06, Jelmer Vernooij <jelmer@samba.org> wrote:
> The attached patch allows the callbacks in svn_ra_callbacks2_t() to be
> implemented in Python. This is one of the things required in order to
> get svn_ra_commit() working from Python.

Could you update our test suite with a few new tests to make sure your
code works? For example, you could register a few callback functions
in Python and make sure they get called with appropriate arguments.

See below for a few important fixes.

>[...]
> +DECLARE_SWIG_CONSTRUCTOR(string, svn_string_dup)
> [...]
> + make_ob_string, value,
> [...]
>+ make_ob_string, value,

You're converting SVN strings into SWIG pointers, here. Instead, we
should convert svn strings into real Python strings using
PyString_FromStringAndSize((void *)s->data, s->len). See
convert_svn_string_t for an example.

>+ if (svn_swig_ConvertPtrString(result, (void **)fp,
>+ "apr_file_t *") == -1)
>+ {
>+ err = type_conversion_error
>+ ("apr_file_t *");
>+ Py_DECREF(result);
>+ }

You're converting from SWIG "apr_file_t *" pointers into C "apr_file_t
*" pointers, here. It's good that we can handle apr_file_t * pointers,
but we should also be able to handle regular Python file objects. See
svn_swig_py_make_file for some example code which works with Python
file objects.

>+ if (svn_swig_ConvertPtrString(result, (void **)value,
>+ "svn_value_t *") == -1)
>+ {
>+ err = type_conversion_error("svn_value_t *");
>+ Py_DECREF(result);
>+ }

This code converts Python "svn_string_t *" pointers into C
"svn_string_t *" pointers. It'd be much better if we let users supply
Python strings instead of pointers. You can convert from Python
strings into "svn_string_t *" objects using the
make_svn_string_from_ob function.

>+ if ((result = PyObject_CallMethod(callbacks,
>+ "progress",
>+ (char *)"iiO&",
>+ progress, total,
>+ make_ob_pool, pool)) == NULL)
>+ {
>+ err = callback_exception_error();
>+ svn_swig_py_svn_exception(err);
>+ }
>+ else if (result != Py_None)
>+ {
>+ err = callback_bad_return_error("Not None");
>+ Py_DECREF(result);
>+ }
>+

If result == Py_None, there's a memory leak here. If you're going to
throw away a reference, you always need to run Py_DECREF, unless
result == NULL. Py_XDECREF will do the trick here. I noticed this
memory leak in several places, but I only commented on it here.

>+ if (svn_swig_ConvertPtrString(py_auth_baton,
>+ (void **)&((*callbacks)->auth_baton),
>+ "svn_auth_baton_t *") == -1)
>+ {
>+ err = type_conversion_error("svn_auth_baton_t *");
>+ svn_swig_py_svn_exception(err);
>+ Py_DECREF(py_auth_baton);
>+ return;
>+ }

You're converting a SWIG auth baton pointer directly into a C auth
baton here. I see you also implicitly increased the reference count of
auth_baton when you called PyObject_GetAttrString. Oops. You should
Py_DECREF(py_auth_baton) unconditionally so as to prevent memory
leaks.

Also, do we need to check for "-1" specifically? Other code in
swigutil_py.c checks for nonzero values instead.

Thanks for your work on the Python bindings, Jelmer! If you can fix
these issues, and write some tests, I'll be able to commit your patch.

Cheers,

David

-- 
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org