[PATCH v2 0/9] box: rework functions machinery
Vladimir Davydov
vdavydov.dev at gmail.com
Mon Jun 10 12:14:46 MSK 2019
On Thu, Jun 06, 2019 at 03:03:56PM +0300, Kirill Shcherbatov wrote:
> src/box/CMakeLists.txt | 1 +
> src/box/alter.cc | 118 +++++--
> src/box/bootstrap.snap | Bin 4393 -> 4449 bytes
> src/box/call.c | 157 +--------
> src/box/call.h | 14 -
> src/box/errcode.h | 1 +
> src/box/execute.c | 1 +
> src/box/func.c | 564 ++++++++++++++++++++++++++++--
> src/box/func.h | 84 +++--
> src/box/func_def.c | 9 +
> src/box/func_def.h | 43 ++-
> src/box/lua/call.c | 379 ++------------------
> src/box/lua/init.c | 2 +
> src/box/lua/port_lua.c | 318 +++++++++++++++++
> src/box/lua/schema.lua | 38 +-
> src/box/lua/upgrade.lua | 25 +-
> src/box/port.h | 2 +-
> src/box/schema.cc | 47 ++-
> src/box/schema.h | 31 +-
> src/box/schema_def.h | 4 +
> src/lib/core/port.h | 16 +
> src/lua/utils.c | 126 +++++++
> src/lua/utils.h | 19 +
I don't quite like the way you split the code among modules. First, we
try to keep all Lua-related stuff in src/lua or src/box/lua. There are
exceptions (e.g. luaT_module_find located in src/box/func.c), but we try
to avoid them.
I think that the code should be split as follows:
- src/box/func.[hc] should contain definition of the base func class,
C func implementation and module manipulation.
- src/box/lua/call.[ch] should contain implementation of Lua func
class, both for persistent functions and not, and helpers to run Lua
code (eval/call). We might want to rename src/box/lua/call.[hc] to
func.[hc], but I'm not sure it's really necessary at this point.
Also, I don't think it's worth moving sandboxing to util.[hc] - it's
only used to run Lua code so I guess it's okay to leave it in
lua/call along with port_lua and luaT_func_find.
- src/box/call.[hc] should contain helpers to invoke a call request
(CALL/EVAL). CALL code should check permissions, look up function by
name and, if it's found execute, execute it, otherwise call a helper
function defined in lua/func to run the function by name.
A general node: please avoid patching and moving the code at the same
time, except cases when everything you change is names of the moved
functions: this makes the code nearly impossible to review for
correctness.
Here's how I'd try to organize the patch set:
1. Simplify func updates on alter (in a little bit different way than
you did - see my comments to the corresponding patch).
2. Re-factor box_lua_call and box_lua_eval so that they don't take
call_request. I think they should take struct port. Rationale: in
case of a functional index, the user expects to see a tuple with
field names so we should be able to pass not only raw msgpack, but
also a tuple to a Lua call.
3. Introduce base func class to src/box/func.[hc]. Implement func_c in
src/box/func.c and func_lua in src/box/lua/call.c (func_lua_create
should be declared in src/box/lua/call.h so that vtab and other
internals aren't exposed).
4. Add infrastructure to call functions from Lua. The code should live
in src/box/lua/call.c. Please write a separate docbot request for
this one.
5. Add persistent functions. Splitting it in three patches doesn't
really facilitate review IMO: I want to see the complete picture,
including new _func format and sandboxing. BTW I guess is_sandbox
shouldn't be a part of options at this time - it should be a
separate column, like is_deterministic.
I'll post a few more comments re the code in reply to emails with
patches.
Thoughts, objections?
More information about the Tarantool-patches
mailing list