Opened 7 years ago

Closed 6 years ago

#2089 closed enhancement (fixed)

liblwgeom cannot be used through python ctypes

Reported by: brushtyler Owned by: strk
Priority: medium Milestone: PostGIS 2.1.0
Component: liblwgeom Version: trunk
Keywords: Cc: darkpanda

Description

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.

Attachments (2)

ffi-lwgeom.c (1.1 KB) - added by darkpanda 7 years ago.
lwgeom_fix_2089.diff (11.9 KB) - added by brushtyler 6 years ago.
add lwgeom_install_handlers function to set custom handlers

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by brushtyler

Owner: changed from pramsey to strk

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.

comment:2 Changed 7 years ago by strk

Type: defectenhancement

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 ?

comment:3 Changed 7 years ago by darkpanda

Cc: darkpanda added

comment:4 in reply to:  2 Changed 7 years ago by 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.

comment:5 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by 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.

Changed 7 years ago by darkpanda

Attachment: ffi-lwgeom.c added

comment:7 Changed 6 years ago by 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.

comment:8 Changed 6 years ago by 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.

comment:9 in reply to:  8 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by 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 :)

comment:11 in reply to:  10 Changed 6 years ago by 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...

comment:12 Changed 6 years ago by 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 :)

Changed 6 years ago by brushtyler

Attachment: lwgeom_fix_2089.diff added

add lwgeom_install_handlers function to set custom handlers

comment:13 Changed 6 years ago by brushtyler

Second attempt.

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

comment:14 Changed 6 years ago by 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

comment:15 Changed 6 years ago by 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...

comment:16 Changed 6 years ago by 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’

comment:17 in reply to:  15 Changed 6 years ago by 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.

comment:18 in reply to:  16 Changed 6 years ago by 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.

comment:19 Changed 6 years ago by 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

comment:20 Changed 6 years ago by 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.

comment:21 Changed 6 years ago by 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.

comment:22 Changed 6 years ago by brushtyler

pull request 4 opened on the PostGIS github repo.

comment:23 Changed 6 years ago by brushtyler

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

comment:24 Changed 6 years ago by 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)

comment:25 Changed 6 years ago by strk

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

comment:26 Changed 6 years ago by strk

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.