Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>,
	tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module
Date: Mon, 5 Apr 2021 18:04:41 +0200	[thread overview]
Message-ID: <8d3a4d20-2887-7423-777f-2af808b65495@tarantool.org> (raw)
In-Reply-To: <20210402123420.885834-7-gorcunov@gmail.com>

Thanks for the patch!

See 10 comments below.

> @TarantoolBot document
> Title: box.lib module
> 
> Overview
> ========
> 
> `box.lib` module provides a way to create, delete and execute
> `C` procedures from shared libraries. Unlike `box.schema.func`
> methods the functions created with `box.lib` help are not persistent
> and live purely in memory. Once a node get turned off they are
> vanished. An initial purpose for them is to execute them on
> nodes which are running in read-only mode.
> 
> Module functions
> ================
> 
> `require('box.lib').load(path) -> obj | error`

1. Implementation in C should allow not to call 'require()' at all. For
example, you don't call require('box.tuple') to use the tuples. The same
for lib.

However I don't know what is the current rule for the new modules - force to
call require() or not. Please, ask Mons what should we do. In case we need to
force to call require() - I have no idea how to do that TBH, since the module
is in C really, not in Lua. It is loaded as a part of the executable file
anyway.


2. Why can't I use box.lib without calling box.cfg{}? I does not depend on it
anyhow. For example, box.tuple works fine, require() on .so files works fine,
even ffi.load() works fine without box.cfg{}. What is the problem with box.lib?

> Unloads a module. Returns `true` on success, otherwise an error
> is thrown. Once the module is unloaded one can't load new
> functions from this module instance.
> 
> Possible errors:
> 
> - IllegalParams: a module is not supplied.
> - IllegalParams: a module is already unloaded.
> 
> Example:
> 
> ``` Lua
> m = require('box.lib').load('path/to/library')
> --
> -- do something with module
> --
> m:unload()
> ```
> 
> If there are functions from this module referenced somewhere
> in other places of Lua code they still can be executed because
> the module continue sitting in memory until the last reference
> to it is closed.
> 
> If the module become a target to the Lua's garbage collector
> then unload is called implicitly.
> 
> module:load(name) -> obj | error`

3. You need to add ` in the beginning too. Otherwise the document
structure would be screwed.

> ---
>  src/box/CMakeLists.txt |   1 +
>  src/box/lua/box_lib.c  | 590 +++++++++++++++++++++++++++++++++++++++++
>  src/box/lua/box_lib.h  |  25 ++

4. It is already in box/ folder, you don't need box_ prefix for
the files. For example, we don't have box_tuple.h/box_tuple.c. We
have just tuple.c and tuple.h, and so on.

>  src/box/lua/init.c     |   2 +
>  test/box/misc.result   |   1 +
>  5 files changed, 619 insertions(+)
>  create mode 100644 src/box/lua/box_lib.c
>  create mode 100644 src/box/lua/box_lib.h
> 
> diff --git a/src/box/lua/box_lib.c b/src/box/lua/box_lib.c
> new file mode 100644
> index 000000000..ce2ef8902
> --- /dev/null
> +++ b/src/box/lua/box_lib.c
> @@ -0,0 +1,590 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <lua.h>
> +
> +#include "box/error.h"
> +#include "box/port.h"
> +
> +#include "tt_static.h"
> +
> +#include "assoc.h"
> +#include "box_lib.h"
> +#include "diag.h"
> +#include "module_cache.h"
> +
> +#include "lua/utils.h"
> +
> +/**
> + * Function descriptor.
> + */
> +struct box_module_func {
> +	/** C function to call. */
> +	struct module_func mf;

5. You could call it 'base' like we normally do when inherit
structures.

> +	/** Number of references. */
> +	int64_t refs;
> +	/** Length of functon name in @a key. */
> +	size_t sym_len;
> +	/** Length of @a key. */
> +	size_t len;
> +	/** Function hash key. */
> +	char key[0];
> +};

<...>

> +/** Handle __index request for a module object. */
> +static int
> +lbox_module_index(struct lua_State *L)
> +{
> +	lua_getmetatable(L, 1);
> +	lua_pushvalue(L, 2);
> +	lua_rawget(L, -2);
> +	if (!lua_isnil(L, -1))
> +		return 1;

6. What is happening here in these 5 lines?

> +
> +	struct module *m = get_udata(L, uname_lib);
> +	if (m == NULL) {
> +		lua_pushnil(L);
> +		return 1;
> +	}

<...>

> +
> +/**
> + * Load a function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and either returns a cached function or
> + * creates a new function object.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - SystemError: unable to open a module due to a system error.
> + * - ClientError: a module does not exist.
> + * - OutOfMemory: unable to allocate a module.
> + *
> + * @returns module object on success or throws an error.
> + */
> +static int
> +lbox_module_load_func(struct lua_State *L)
> +{
> +	const char *method = "function = module:load";
> +	const char fmt_noname[] = "Expects %s(\'name\') but no name passed";

7. Why do you randomly jump between const char * and const char []?
Please, use const char * to be consistent, and because it looks simpler.

> +
> +	if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) {

8. Here you use lua_isstring(), in some places you use
lua_type() == LUA_TSTRING. Please, be consistent, and choose one way.

> +		diag_set(IllegalParams, fmt_noname, method);
> +		return luaT_error(L);
> +	}
> +
> +	struct module *m = get_udata(L, uname_lib);
> +	if (m == NULL) {
> +		const char *fmt =
> +			"Expects %s(\'name\') but not module object passed";
> +		diag_set(IllegalParams, fmt, method);
> +		return luaT_error(L);
> +	}
> +
> +	size_t sym_len;
> +	const char *sym = lua_tolstring(L, 2, &sym_len);
> +
> +	if (sym_len < 1) {
> +		diag_set(IllegalParams, fmt_noname, method);
> +		return luaT_error(L);
> +	}
> +
> +	/*
> +	 * Functions are bound to a module symbols, thus
> +	 * since the hash is global it should be unique
> +	 * per module. The symbol (function name) is the
> +	 * last part of the hash key.
> +	 */
> +	const char *key = tt_sprintf("%p.%s.%s", (void *)m,
> +				     m->package, sym);

9. I wouldn't use the static buffer in non-trivial code, and
never in Lua and Lua C code. Especially after the recently
discovered bug.

Firstly, something inside box_module_func_new() might also try
to use the static buffer and would overwrite it. Secondly, deep
inside it accesses Lua to find package.* content, and that might
trigger Lua GC, which also might use the static buffer for
anything.

> +	size_t len = strlen(key);
> +
> +	struct box_module_func *cf = cache_find(key, len);
> +	if (cf == NULL) {
> +		cf = box_module_func_new(m, key, len, sym_len);
> +		if (cf == NULL)
> +			return luaT_error(L);
> +	} else {
> +		box_module_func_ref(cf);
> +	}
> +
> +	new_udata(L, uname_func, cf);
> +	return 1;
> +}
> +
> +/**
> + * Unload a function.
> + *
> + * Take a function object from the caller stack @a L and unload it.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: the function is not supplied.
> + * - IllegalParams: the function already unloaded.
> + *
> + * @returns true on success or throwns an error.
> + */
> +static int
> +lbox_func_unload(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1) {
> +		diag_set(IllegalParams, "Expects function:unload()");
> +		return luaT_error(L);
> +	}
> +
> +	struct box_module_func *cf = get_udata(L, uname_func);
> +	if (cf == NULL) {
> +		diag_set(IllegalParams, "The function is unloaded");
> +		return luaT_error(L);
> +	}
> +
> +	set_udata(L, uname_func, NULL);

10. By having get_udata and set_udata helpers you force yourself to
do double-check for the cdata on each pair of these calls. While
you could get void** first time, and simply *ptr = NULL it instead
of set_udata(NULL). Up to you, just a note.

  reply	other threads:[~2021-04-05 16:04 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-02 12:34 [Tarantool-patches] [PATCH v20 0/7] box: implement box.lib Lua module Cyrill Gorcunov via Tarantool-patches
2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created Cyrill Gorcunov via Tarantool-patches
2021-04-05  9:28   ` Serge Petrenko via Tarantool-patches
2021-04-05  9:50     ` Cyrill Gorcunov via Tarantool-patches
2021-04-05 10:13       ` Serge Petrenko via Tarantool-patches
2021-04-05 15:45   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-06  7:44     ` Cyrill Gorcunov via Tarantool-patches
2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 2/7] box/func: module_reload -- drop redundant argument Cyrill Gorcunov via Tarantool-patches
2021-04-05 10:23   ` Serge Petrenko via Tarantool-patches
2021-04-05 10:26     ` Serge Petrenko via Tarantool-patches
2021-04-05 10:31       ` Cyrill Gorcunov via Tarantool-patches
2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore Cyrill Gorcunov via Tarantool-patches
2021-04-05 10:53   ` Serge Petrenko via Tarantool-patches
2021-04-05 11:26     ` Cyrill Gorcunov via Tarantool-patches
2021-04-05 11:42       ` Serge Petrenko via Tarantool-patches
2021-04-05 15:47   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-06  8:38     ` Cyrill Gorcunov via Tarantool-patches
2021-04-06 20:02       ` Vladislav Shpilevoy via Tarantool-patches
2021-04-06 20:42         ` Cyrill Gorcunov via Tarantool-patches
2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem Cyrill Gorcunov via Tarantool-patches
2021-04-05 12:34   ` Serge Petrenko via Tarantool-patches
2021-04-05 15:52   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-06 14:33     ` Cyrill Gorcunov via Tarantool-patches
2021-04-06 20:09       ` Vladislav Shpilevoy via Tarantool-patches
2021-04-06 22:05         ` Cyrill Gorcunov via Tarantool-patches
2021-04-06 23:43           ` Vladislav Shpilevoy via Tarantool-patches
2021-04-07  7:03             ` Cyrill Gorcunov via Tarantool-patches
2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 5/7] box/schema.func: switch to new module api Cyrill Gorcunov via Tarantool-patches
2021-04-05 15:56   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module Cyrill Gorcunov via Tarantool-patches
2021-04-05 16:04   ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-04-07 16:59     ` Cyrill Gorcunov via Tarantool-patches
2021-04-07 20:22       ` Cyrill Gorcunov via Tarantool-patches
2021-04-07 20:28         ` Vladislav Shpilevoy via Tarantool-patches
2021-04-07 20:37           ` Cyrill Gorcunov via Tarantool-patches
2021-04-07 20:45             ` Cyrill Gorcunov via Tarantool-patches
2021-04-07 21:04               ` Vladislav Shpilevoy via Tarantool-patches
2021-04-02 12:34 ` [Tarantool-patches] [PATCH v20 7/7] test: add box.lib test Cyrill Gorcunov via Tarantool-patches
2021-04-05 16:04   ` Vladislav Shpilevoy via Tarantool-patches
2021-04-05 15:45 ` [Tarantool-patches] [PATCH v20 0/7] box: implement box.lib Lua module Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8d3a4d20-2887-7423-777f-2af808b65495@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v20 6/7] box: implement box.lib module' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox