Re: [PATCH] svn_client_ctx_t callback fixes, mark 2

On 5/28/06, Walter Mundt <emage@spamcop.net> wrote:
> After some discussion with maxb on IRC and a bit more work, here is a
> new-and-hopefully-improved patch. Main changes are a warning about the
> dangers of the PY_AS_VOID typemap, using #ifdef's around that typemap to
> match with Jelmer's patch, and a new test of the client.log3() function.
> [...]
> Provide direct access to certain libsvn_swig_py "thunk" function
> pointers to Python so that Python code can properly set fields in
> svn_client_ctx_t structures.

Nice work, Walter! I've included some comments on your patch below.

> + Mapper to automatically turn Python objects into void* batons on assignment
> +*/
> +
> +#ifdef SWIGPYTHON
> +/* NOTE: Use of this typemap can be dangerous, because it doesn't INCREF
> + the object. All objects passed via this template MUST have a
> + reference kept alive elsewhere in Python code. */
> +%typemap(in) void *PY_AS_VOID {
> + if ($input == Py_None) {
> + $1 = NULL;
> + } else {
> + $1 = (void *)$input;
> + }
> +}
> +#endif

You're right: This typemap is dangerous! If a reference to this baton
isn't kept alive elsewhere in the bindings, Python will crash.

To make this typemap safe, I recommend you keep track of the $input
baton pointer inside the current pool. This way, you can ensure that
the lifetime of the Python baton is the same as its associated pool.

For examples of this technique in action, see the Ruby bindings. The
Ruby bindings maintain a "batons" array inside each pool which helps
keep the batons alive for the lifetime of the pool.

If possible, I'd prefer if we can keep our baton tracking code
generic. Ideally, we should be able to keep our batons alive by simply
adding references to them into our current _global_svn_swig_py_pool
variable. You should test this code by testing what happens when the
pool is destroyed -- the baton should only be destroyed when the
associated pool is destroyed.

Note that the value of _global_svn_swig_py_pool differs depending on
what typemap you are currently in. The generated SWIG code
automatically generates new pools for each function which are in turn
tracked by the objects we send back to Python.

> + self.client_ctx.log_msg_func2 = client.svn_swig_py_get_commit_log_func
> + # nasty refcount issue -- to be fixed in higher-level bindings
> + self.boundLogMsgFunc = self.log_message_func
> + self.client_ctx.log_msg_baton2 = self.boundLogMsgFunc
Nasty. Ideally, we should be able to get rid of this code if we keep
track of batons in the current pool.

> + self.change_auther = None
Oops. This should be "self.change_author".

> + def test_mkdir_url(self):
> + """Test svn_client_mkdir2 on a file:// URL"""
> + dir = urljoin(self.repos_url+"/", "dir1")
> +
> + commit_info = client.mkdir2((dir,), self.client_ctx)
> + self.assertEqual(commit_info.revision, 13)
> + self.assertEqual(self.log_message_func_calls, 1)
>
> + def test_log3_url(self):
> + """Test svn_client_log3 on a file:// URL"""
> + dir = urljoin(self.repos_url+"/", "trunk/dir1")
> +
> + start = core.svn_opt_revision_t()
> + end = core.svn_opt_revision_t()
> + core.svn_opt_parse_revision(start, end, "4:0")
> + client.log3((dir,), start, start, end, 1, True, False, self.log_receiver,
> + self.client_ctx)
> + self.assertEqual(self.change_author, "john")
> + self.assertEqual(self.log_message, "More directories.")
> + self.assertEqual(len(self.changed_paths), 3)
> + for dir in ('/trunk/dir1', '/trunk/dir2', '/trunk/dir3'):
> + self.assertTrue(self.changed_paths.has_key(dir))
> + self.assertEqual(self.changed_paths[dir].action, 'A')
Nice test case!

> +/* provide Python with access to some thunks. */
> +%constant svn_cancel_func_t svn_swig_py_cancel_func;
> +%constant svn_client_get_commit_log2_t svn_swig_py_get_commit_log_func;
Nice! Very slick and simple. I wasn't aware of SWIG's %constant
directive, so I read up on it at:
  http://www.swig.org/Doc1.3/Python.html#Python_nn17

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