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

[Jelmer Vernooij <je...@vernstok.nl>]

Hi David,

David James wrote:
> On 5/25/06, Jelmer Vernooij <je...@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.
ra_local doesn't actually call these callbacks, which makes it very hard
to write tests using the file:// RA backend. Apparently ra_dav uses them
most often, but there are no Python tests that use it. I have used them
successfully here using other protocols though.

> 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.
Fixed.

> 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.
Fixed.

> 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.
Fixed.

> 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.
Fixed.

> 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.
That should be ok, as the functions on svn.client that return auth_batons return opaque pointers. I've fixed the 
reference.

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

Here's the summary again:

[[[

Allow callbacks in svn_ra_callbacks2_t() to be implemented in Python.

 * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c

   (svn_swig_py_setup_ra_callbacks): Fill in callback function pointers 
   and auth_baton

   (ra_callbacks_open_tmp_file,
    ra_callbacks_get_wc_prop,
    ra_callbacks_set_wc_prop,
    ra_callbacks_push_wc_prop,
    ra_callbacks_invalid_wc_props,
    ra_callbacks_progress_funcs): Add wrappers for Python callbacks

   (make_ob_string): Add function
  
]]]


Cheers,

Jelmer

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

[David James <dj...@collab.net>]

On 7/5/06, Jelmer Vernooij <je...@samba.org> wrote:
> > Ah, OK. Jelmer, could you provide some example test scripts which use
> > these functions so that I can test them myself?
> I've attached an example script that does a checkout, which crashes
> without this patch.
Thanks!

> I've attached a patch that contains the subset of functions that are
> proven to work now using the attached example script.
>
> [[[
> Allow some callbacks in svn_ra_callbacks2_t() to be implemented in
> Python.
>
>  * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
>
>    (svn_swig_py_setup_ra_callbacks): Fill in callback function pointer
>    for open_tmp_file and auth_baton.
>
>    (ra_callbacks_open_tmp_file): Add wrapper for Python callbacks
>
> ]]]

+1. Thanks for following up! This is a nice patch. Could you reference
this thread from your commit message? Mention that you included a test
script in this mail thread, so that anybody who is interested can add
a real test later.

The mail thread URL is
http://svn.haxx.se/dev/archive-2006-07/0126.shtml and your test-dav.py
is accessible from that URL.

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

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

[Jelmer Vernooij <je...@samba.org>]

On Mon, 2006-06-12 at 11:53 -0700, David James wrote:
> On 6/11/06, Jelmer Vernooij <je...@vernstok.nl> wrote:
> > > 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.
> > ra_local doesn't actually call these callbacks, which makes it very hard
> > to write tests using the file:// RA backend. Apparently ra_dav uses them
> > most often, but there are no Python tests that use it. I have used them
> > successfully here using other protocols though.
> 
> Ah, OK. Jelmer, could you provide some example test scripts which use
> these functions so that I can test them myself?
I've attached an example script that does a checkout, which crashes
without this patch.

> In all new functions: Use Py_XDECREF instead of Py_DECREF.
> Py_XDECREF handles NULL values safely. Only use Py_DECREF if you are
> absolutely sure that the variable in question will not be NULL.
Fixed.

I've attached a patch that contains the subset of functions that are
proven to work now using the attached example script.

[[[
Allow some callbacks in svn_ra_callbacks2_t() to be implemented in
Python.

 * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c

   (svn_swig_py_setup_ra_callbacks): Fill in callback function pointer 
   for open_tmp_file and auth_baton.

   (ra_callbacks_open_tmp_file): Add wrapper for Python callbacks
  
]]]

Cheers,

Jelmer

-- 
Jelmer Vernooij <je...@samba.org> - http://samba.org/~jelmer/

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

[Madan U Sreenivasan <ma...@collab.net>]

On Tue, 13 Jun 2006 00:23:44 +0530, David James <dj...@collab.net> wrote:

> On 6/11/06, Jelmer Vernooij <je...@vernstok.nl> wrote:
>> > 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.
>> ra_local doesn't actually call these callbacks, which makes it very hard
>> to write tests using the file:// RA backend. Apparently ra_dav uses them
>> most often, but there are no Python tests that use it. I have used them
>> successfully here using other protocols though.
>
> Ah, OK. Jelmer, could you provide some example test scripts which use
> these functions so that I can test them myself?
>
> At some point, we should add a 'dav-autocheck-swig-py' target, similar
> to the regular dav-autocheck target, which checks the 'swig-py'
> bindings instead. Madan, would you be interested in working on this?

Of course, I'm.

But sadly, am stuck trying to get wind of swig basics... am going round in  
circles learning swig and how we use it :-\ . Any pointers to simple swig  
tutorials would be of great help.

Regards,
Madan.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

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

[David James <dj...@collab.net>]

On 6/11/06, Jelmer Vernooij <je...@vernstok.nl> wrote:
> > 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.
> ra_local doesn't actually call these callbacks, which makes it very hard
> to write tests using the file:// RA backend. Apparently ra_dav uses them
> most often, but there are no Python tests that use it. I have used them
> successfully here using other protocols though.

Ah, OK. Jelmer, could you provide some example test scripts which use
these functions so that I can test them myself?

At some point, we should add a 'dav-autocheck-swig-py' target, similar
to the regular dav-autocheck target, which checks the 'swig-py'
bindings instead. Madan, would you be interested in working on this?

> > 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.
> Fixed.

Thanks for the fixes, Jelmer! Your new patch looks quite good. I tried
to commit it but I found a few more issues which I'd like us to fix
first. These fixes are fairly straightforward.

1. In all new functions: Use Py_XDECREF instead of Py_DECREF.
Py_XDECREF handles NULL values safely. Only use Py_DECREF if you are
absolutely sure that the variable in question will not be NULL.

2. In ra_callbacks_set_wc_prop and ra_callbacks_push_wc_prop: If
PyString_FromStringAndSize returns NULL, we should return an
appropriate exception to Python.

3. In svn_swig_py_setup_ra_callbacks: If PyObject_GetAttrString
returns NULL, we should return an appropriate exception to Python.

I've also noticed a complex scoping issue which results from your
patch. In svn_swig_py_setup_ra_callbacks, we should ideally tie the
scope of 'py_auth_baton' and 'py_callbacks' to the RA session returned
by svn_ra_open. We need to tie the scope of these two objects together
so that the Python garbage collector will not destroy these objects
while Subversion still needs them.

1. In the typemap for svn_ra_callbacks2_t in svn_ra.i, pass the
current value of _global_svn_swig_py_pool into
svn_swig_py_setup_ra_callbacks. By doing this, you tell
svn_swig_py_setup_ra_callbacks which pool you are using to initialize
the ra session. (Tricky note: The SWIG-generated wrapper for
svn_ra_open defines a local variable named _global_svn_swig_py_pool
which shadows the global variable of the same name. This local
variable can only be accessed within SWIG typemaps, and cannot be
accessed directly from swigutil_py.c.)

2. From svn_swig_py_setup_ra_callbacks, mark py_callbacks and
py_auth_baton as owned by _global_svn_swig_py_pool. You can do this by
calling PyObject_CallMethod(py_pool, addOwnedRef, objectTuple,
py_callbacks) and PyObject_CallMethod(py_pool, addOwnedRef,
objectTuple, py_auth_baton). You don't need to worry about calling
removeOwnedRef, like Walter did, because you are not replacing
anything in the session -- you are creating a brand new session
object.

3. If the calls to PyObject_CallMethod fail, don't forget to return an
appropriate exception to Python. Otherwise, all is good.

Thanks for your great work, Jelmer! If you can fix the simple issues
in your next patch, and provide a few example test scripts, I think it
will be ready to commit. If you could fix the complex scoping issues
too, that would be great, but I can still commit your patch without
fixes for them.

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