liblwgeom cannot be used through python ctypes

[brushtyler]

I'm trying to access liblwgeom functionalities through python ctypes, but I get an error because lwgeom_init_allocators is not defined.

>>> ctypes.cdll.LoadLibrary("/usr/local/lib/liblwgeom.so")
Traceback (most recent call last):
    ...
    self._handle = _dlopen(self._name, mode)
OSError: /usr/local/lib/liblwgeom.so: undefined symbol: lwgeom_init_allocators

In fact programs want to use the library must define lwgeom_init_allocators() function.

[brushtyler]

Perhaps it would be better to add two methods (one for memory managers and the other one for error handlers) that takes function pointers and initialize the required stuff.

Whether not called (or even passing NULL values as arguments) the default functions will be used.

[strk]

Agreed, we want to do that. postgis and pgraster libs already have their initialization code that could install those routines too...

Feel like working on a patch ?

[brushtyler]

Replying to strk:

postgis and pgraster libs already have their initialization code that could install those routines too...

are you talking about the rt_init_allocators function? That one is called when the raster lib tries to allocate something, so I suppose is not the right place to call the "lwgeom_set_handlers" function because we don't know if lwgeom needs to allocate something before the rt_init_allocators is called.

BTW, I found that the RTLD_LAZY mode can be used to avoid it raises an error, though python ctypes doesn't export that name... I should use "mode=1" opening the library.

Feel like working on a patch ?

Yes, but I've to understand what should be changed. Surely I must add a lwgeom_set_handlers function.

[strk]

I'm talking about _PG_init. You have one in postgis_module.c The one for raster lib isn't there, so should be added.

And of course you need to add the lwgeom_set_handlers function.

[darkpanda]

After seeing this ticket I decided to see what it would take to write a similar bit of code using ruby and its FFI extensions, which is basically the equivalent of python's ctypes module. I ended up having to write a small shim that implemented lwgeom_init_allocators and set up handlers for errors and notices. The whole thing clocked in at around 40 lines of code in C and the rest can be done in pure ruby at this point.

Perhaps what could be added on the lwgeom side is a set of functions that allow you to set the pointers to lwalloc_var, lwrealloc_var, lwfree_var, lwnotice_var and lwerror_var as necessary and have them point to the defaults initially. This way you wouldn't need to implement lwgeom_set_handlers directly but could set them via a initialization function (or functions). GEOS does something similar to this with its initGEOS function for setting its error and notice handlers, for instance.

I'll attach the shim I wrote for ruby at any rate as an example. Basically I just load that in and let the ruby code do the rest via FFI.

[brushtyler]

I've added a patch to fix this problem.

The patch does the following:

• adds a lwgeom_set_handlers function to set custom handlers (both memory management and message reporter functions), it replaces the lwgeom_init_allocators function,

• moves the module part of raster/rt_pg/rt_pg.c to the new file raster/rt_pg/pg_module.c,

• and, of course, makes the code working with the new lwgeom_set_handlers function.

Please, review my patch and apply it if you think it's good enough. Otherwise, opinions on how to improve it are welcome.

[strk]

Trying a build and testsuite run with this patch applied:

cu_tester.c:97:31: error: ‘cu_errorreporter’ undeclared (first use in this function)

I think the setter function to return the previous handler, so to allow for pluggable memory debugger or disk loggers. Or there should be another function to extract them. Current API does give direct access to the handler variables.

[brushtyler]

Replying to strk:

cu_tester.c:97:31: error: ‘cu_errorreporter’ undeclared (first use in this function)

I've attached the wrong patch... sorry. I'm attaching the correct one.

I think the setter function to return the previous handler, so to allow for pluggable memory debugger or disk loggers. Or there should be another function to extract them. Current API does give direct access to the handler variables.

The setter function it's based on rt_set_handlers which doesn't do it. BTW I can add a getter for each handler or change the current lwgeom_set_handlers implementation if you prefer.

[strk]

Still no luck with the second patch:

rt_module.c:58:1: error: redefinition of ‘Pg_magic_func’
rt_module.c:23:1: note: previous definition of ‘Pg_magic_func’ was here
rt_module.c:65:1: error: redefinition of ‘_PG_init’
rt_module.c:30:1: note: previous definition of ‘_PG_init’ was here

Adding getters may be subject to race conditions (something else changes the handler between the getter call and the setter call). Not sure how a flexible and stable interface could look like for atomically setting the handlers. Maybe with input/output parameters (although that'd force the caller to provide non-const pointers). Or splitting into multiple functions each setting a single handler (although it wouldn't make much sense to register an allocator without a deallocator). Well, let's focus on a working build first :)

[brushtyler]

Replying to strk:

Still no luck with the second patch:

The second patch? Which one? I was waiting for your reply about getters before attaching it...

[strk]

Oh, I thought you overwrote it! Dunno why I'm having a different error now...

Well anyway the comment is above. See if you can get any inspiration for an atomic getter/setter.

I was thinking that maybe the function could take a pointer to a structure and the structure would contain all needed pointers and would be written all old pointers. That way it's also easy to extend by adding more elements to the structure. It would be helpful to have a structure initializer, in that case, so that the calling code doesn't need to change shall we add more configurable pointers in the future...

But then again, consider all of this as brainstorming, don't let it block you and feel free to attach what you have so far :)

[brushtyler]

add lwgeom_install_handlers function to set custom handlers

[brushtyler]

Second attempt.

On my Ubuntu12.10 make check ends without any failure.

[strk]

rt_module.c:58:1: error: redefinition of ‘Pg_magic_func’
rt_module.c:23:1: note: previous definition of ‘Pg_magic_func’ was here
rt_module.c:65:1: error: redefinition of ‘_PG_init’
rt_module.c:30:1: note: previous definition of ‘_PG_init’ was here
rt_module.c:93:1: error: redefinition of ‘Pg_magic_func’
rt_module.c:23:1: note: previous definition of ‘Pg_magic_func’ was here
rt_module.c:100:1: error: redefinition of ‘_PG_init’
rt_module.c:30:1: note: previous definition of ‘_PG_init’ was here

The above as the result of:

./autogen.sh && ../configure.local && make clean all check RUNTESTFLAGS=-v

[strk]

Looking at the file, it seems to be a problem with applying the patch. Want to use a pull request on github ? It will do fine...

[strk]

Tried again after wiping out rt_module.c and I get:

In file included from cu_buildarea.c:14:0:
cu_tester.h:22:35: error: unknown type name ‘va_list’

[brushtyler]

Replying to strk:

Want to use a pull request on github ? It will do fine...

I prefer git to svn, if you say the postgis github mirror can be used I'll open a pull request there.

[brushtyler]

Replying to strk:

Tried again after wiping out rt_module.c and I get:

In file included from cu_buildarea.c:14:0:
cu_tester.h:22:35: error: unknown type name ‘va_list’

That's crazy, no errors here... sigh.

BTW the missing row should be

#include <stdarg.h>

as it includes the va_list definition.

[strk]

brushtyler: do you have raster support build enabled ?

cu_tester.c:243:6: warning: no previous prototype for ‘lwgeom_init_allocators’ [-Wmissing-prototypes]
cu_tester.c: In function ‘lwgeom_init_allocators’:
cu_tester.c:244:2: error: ‘lwalloc_var’ undeclared (first use in this function)
cu_tester.c:244:2: note: each undeclared identifier is reported only once for each function it appears in
cu_tester.c:245:2: error: ‘lwrealloc_var’ undeclared (first use in this function)
cu_tester.c:246:2: error: ‘lwfree_var’ undeclared (first use in this function)
cu_tester.c:247:2: error: ‘lwnotice_var’ undeclared (first use in this function)
cu_tester.c:248:2: error: ‘lwerror_var’ undeclared (first use in this function)
make[3]: *** [cu_tester.o] Error 1

I'll wait for the pull request

[brushtyler]

strk your branch is a mess or the attached patch is the previous one (the wrong one, though I removed and recreated it by running svn diff).

That's because the lwgeom_init_allocators function was present in the old/wrong patch. Instead the new one doesn't contains such a function, on the contrary it removes that at all!

I'm on the train right now, I cannot check the patch is the good one. I fork the GH repo when I'll arrive at home, but in the meanwhile I'd really appreciate if you could look at it: just check the patch doesn't add a row like lwgeom_init_allocators, otherwise wait for the pull request on github (probably tomorrow).

BTW the patch seems to work fine here, I do not get any error neither with both raster and topology enabled.

[brushtyler]

I forgot the patch is a diff the trac can display w/o the need to download it. And it's the good one! so the problem you get is due to your branch state I suppose.

[brushtyler]

​pull request 4 opened on the PostGIS github repo.

[brushtyler]

I've updated the pull request, now raster's unit test passes without any issue.

[strk]

a few more issues:

1. Do not add an rt_module.c file, the only file in rasterland dealing with postgresql should be rt_pg.c
2. Reduce the changes to the least possible files. In particular try to avoid modifying header files unless strictly necessary. I think neither cu_tester.h nor rt_pg.h nor lwgeom_pg.h need any change (the _init_allocators functions could be module-statics)

[strk]

Sorry, now looking at ​https://github.com/postgis/postgis/pull/6 which seems to have solved my issues above.

[strk]

In r10937 I've committed a slightly modified version of your patch. In particular the lwgeom_set_handlers function considers NULL parameter values as a request to NOT change the current handlers (rather than replacing them with the default).

We're still missing the support to query the old handlers, anyway we can use another ticket for that.