Re: [Tarantool-patches] [PATCH v21 5/6] box: implement box.lib module

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Sun, Apr 11, 2021 at 05:38:51PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Please, try to respond to my comments in the original email,
> explicitly. When and how you fixed them, or why did not. Not
> with promises like "will do", "ok", and so on, they don't help
> much.
> 
> Otherwise I see you tend to miss some of the comments. Also I
> don't remember all of my comments, so when you don't respond to
> them, I need to go to my old email, and match the comments
> with the places you was supposed to fix, and it takes time.
> 
> See how Sergey and Mergen send their patches for examples.
> 
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#during-the-review
> 
> See 4 comments below.

I fixed the change log. And here is a diff on top, please take a look.
---
Subject: [PATCH] box.lib fix

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/lua/lib.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/src/box/lua/lib.c b/src/box/lua/lib.c
index 5fb616d04..2181b9030 100644
--- a/src/box/lua/lib.c
+++ b/src/box/lua/lib.c
@@ -97,12 +97,22 @@ cache_put(struct box_module_func *cf)
 		.hash	= mh_strn_hash(cf->key, cf->len),
 		.val	= cf,
 	};
-	mh_int_t e = mh_strnptr_put(func_hash, &nd, NULL, NULL);
+
+	struct mh_strnptr_node_t prev;
+	struct mh_strnptr_node_t *prev_ptr = &prev;
+
+	mh_int_t e = mh_strnptr_put(func_hash, &nd, &prev_ptr, NULL);
 	if (e == mh_end(func_hash)) {
 		diag_set(OutOfMemory, sizeof(nd), "malloc",
 			 "box.lib: hash node");
 		return -1;
 	}
+
+	/*
+	 * Just to make sure we haven't replaced something,
+	 * the entries must be explicitly deleted.
+	 */
+	assert(prev_ptr == NULL);
 	return 0;
 }
 
@@ -383,7 +393,15 @@ lbox_module_load_func(struct lua_State *L)
 
 	size_t sym_len;
 	const char *sym = lua_tolstring(L, 2, &sym_len);
-	const size_t max_sym_len = 512;
+
+	/*
+	 * C standard requires at least 63 significant
+	 * initial characters, though it advises to not
+	 * impose limits. Lets make the max identifier
+	 * big enough to keep longest id, which is hardly
+	 * be bigger than 256 symbols.
+	 */
+	const size_t max_sym_len = 256;
 
 	if (sym_len < 1) {
 		diag_set(IllegalParams, fmt_noname, method);
@@ -400,13 +418,12 @@ lbox_module_load_func(struct lua_State *L)
 	 * per module. The symbol (function name) is the
 	 * last part of the hash key.
 	 *
-	 * The key's buffer should be big enough to keep
-	 * the longest package path plus symbol name and
-	 * the pointer.
+	 * Make sure there is enough space for key,
+	 * path and formatting.
 	 */
-	char key[PATH_MAX + 2 * max_sym_len];
+	char key[PATH_MAX + max_sym_len + 32];
 	snprintf(key, sizeof(key), "%p.%s.%s",
-		 (void *)m, m->package, sym);
+		 m, m->package, sym);
 	size_t len = strlen(key);
 
 	struct box_module_func *cf = cache_find(key, len);
-- 
2.30.2