Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added option to pass already created LuaState #224

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

xthebat
Copy link

@xthebat xthebat commented Sep 22, 2022

Hello, I stumble upon limitation of lupa library.

I some cases already created lua_State required to be used. But there is no possibility to pass pointer to lua_State in LuaRuntime. For example Wireshark has embedded Lua interpreter and this interpreter has bindings to Wireshark API. I wanted to write plugin (dissector) using Python because it is much more convenient than Lua for me (a lot libraries, familiar syntax, good IDE, etc). So it is not very hard to start Python interpreter using simple Lua C extensions module but without wrappers like lupa provided, I need to also write these wrappers by myself. So I wrote this simple patch to pass pointer of Lua interpreter into LuaRuntime and it works! I think may be it worth to be added to your library.

Regards

@Le0Developer
Copy link
Contributor

Mind adding some tests?

@scoder
Copy link
Owner

scoder commented Sep 26, 2022

Thanks. I don't think it's that easy, though.

Lupa assumes that it owns the lua_State, and will deallocate it when the LuaRuntime is closed. It doesn't know that the lua_State is actually externally owned and that its lifetime may not be linked to the lifetime of the LuaRuntime. That's one issue.

Another thing is that LuaRuntime.__init__() does some setup of the Lua state and assumes that all of this is safe to do, because the Lua state was just created. That may be an unsafe assumption depending on where the lua_State originated from.

It's also unclear to me what you are passing into the constructor here. Is it the pointer address of the lua_State as a Python int? What if I pass 0 or 123 from Python?

I'm not generally opposed to such an interface, but then it should be provided as a C-level API and not be made publicly available to Python programs.

An example of your own usage could help to understand how you intend this to work.

@xthebat
Copy link
Author

xthebat commented Oct 4, 2022

Sorry for long response, I was bit busy on work.

@scoder

Lupa assumes that it owns the lua_State, and will deallocate it when the LuaRuntime is closed. It doesn't know that the lua_State is actually externally owned and that its lifetime may not be linked to the lifetime of the LuaRuntime. That's one issue.
Another thing is that LuaRuntime.init() does some setup of the Lua state and assumes that all of this is safe to do, because the Lua state was just created. That may be an unsafe assumption depending on where the lua_State originated from.

I've added flag self._lua_allocated in 59a72a3 commit that indicate whether lua_State passed from outside or owned by LuaRuntime. In __cinit__ and in __dealloc__ I've added code to check for call lua.luaL_openlibs and lua.lua_close if LuaRuntime own pointer. It works for me, of course it doesn't mean it works everywhere :)

It's also unclear to me what you are passing into the constructor here. Is it the pointer address of the lua_State as a Python int? What if I pass 0 or 123 from Python?

I've reworked it in 32fd682. Now state is Python int that should contain pointer to lua_State. Yeah, If we pass int that contains invalid pointer then program goes to segfault :( ruthless C world. In last commit no check was done to prevent this situation. I think this thought can help to avoid it for common Python user:

it should be provided as a C-level API and not be made publicly available to Python programs.

But all initialization LuaRuntime done in __cinit__ and lua_State* should passed to it for correct initialization also for case external pointer. To hide such initialization variant in C-level I think LuaRuntime cinit need to be somehow splitted.

An example of your own usage could help to understand how you intend this to work.

When reworked my code I've added Cython proxy module in new commit 32fd682. You could find complete code in embedded.pyx Also I've added code in setup.py to compile it. Compilation produce embedded.cp39-win_amd64.pyd (for win) and this library need to be placed in folder where target Lua interpreter looks for require C-modules and renamed to libpylua.dll (*.so if in Linux). In Lua:

require "libpylua"
python.import("some_package")
python.exec("some_var = some_package.somefunction()")

python.import added in 78f4b16

@scoder
Copy link
Owner

scoder commented Nov 14, 2022

it should be provided as a C-level API and not be made publicly available to Python programs.

But all initialization LuaRuntime done in __cinit__ and lua_State* should passed to it for correct initialization also for case external pointer. To hide such initialization variant in C-level I think LuaRuntime.__cinit__ need to be somehow splitted.

Idea: we can add an internal extension type (@cython.internal) that only wraps a lua_State* and can be passed as an optional argument, like this:

cdef class _ExternalLuaState:
    lua_State* lua_state

cdef api runtime_from_lua_state(lua_State* lua_state):
    cdef _ExternalLuaState arg = _ExternalLuaState.__new__(_ExternalLuaState)
    arg.lua_state = lua_state
    return LuaRuntime(lua_state=arg)

LUA_ENTRY_POINT(libpylua) {
PyImport_AppendInittab("embedded", PyInit_embedded);
Py_Initialize();
PyImport_ImportModule("embedded");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd better add some checks here, e.g.

PyObject *mod = PyImport_ImportModule("embedded");
if (mod==NULL)
{
    PyErr_Print();
}
else
{
     Py_DECREF(mod);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants