From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 06E666EC5D; Thu, 8 Apr 2021 19:41:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 06E666EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617900119; bh=C7NieYdvSbRzPH79JnN1VWn5R9jxUJvvGkBrag4Ir1A=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=Dj7vXzvcx+MjNzZJB4fjLyMQtN4yZ3pTwxEqWr5jjBquZZc9/7i1hS8tWWS4MTVdn xzR9FSHu5WPvwxzs8PuHDXpGumxDIDP8VyAGe+Wue5xA5YJzYW9LOL7zTA2JRP9t0y 6YkEHHg1c6E89gBwCtXlNVzkt23m2MB6dn8GmzeA= Received: from mail-lj1-f169.google.com (mail-lj1-f169.google.com [209.85.208.169]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 840536EC5D for ; Thu, 8 Apr 2021 19:41:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 840536EC5D Received: by mail-lj1-f169.google.com with SMTP id u4so3063721ljo.6 for ; Thu, 08 Apr 2021 09:41:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=/YvovwXsUWoJl64Xwgz3kV1ZVTbF2mJdF/5ThORXF9c=; b=LzprmF4LKPlOR/XTYxa0TKpsTfF8D6kmn3Wqivfg8qNBykMVcrdkxMuZEsEXgvi29v h/9beFxOJkrYTTU6sLmRV+uObDUkpeAKnTKQbLQPODvU4wif59sgAjsK/dBVPyoSHXGI h4aIfJ6LJE13Frf3L4OTf7HUlMUTnVhWQVOHxaj2PiMkkUOCkbpmA+FhTJMjoI+RW8Fa g0YTKvz386+SlVuMTOx0pjCDkuPdrx6sGlMlrfyX1WsbYfRLdBOzFJ/lpmF0AGSPSmMc EQ8zn2twBG+lcIUVwjeot+79b2IiTLo1Z7CfrtnaAHC2TxqroAkkXjpd7M09LbPA3tua +qEQ== X-Gm-Message-State: AOAM532wuCM6mlC27v5K69EZznWTW4UEtrGgICMcjaHkeEngW+dAnuPN HMX09EclBRL2vzU65URpN03ts5pzHqY= X-Google-Smtp-Source: ABdhPJywiuTIw0J6mazXYXHhDF0H/mf52FTlNw9bgb7SO1wh+cfy/1fg3RLKLToUl6pGnjUWAsOSJw== X-Received: by 2002:a2e:854b:: with SMTP id u11mr6614602ljj.318.1617900114782; Thu, 08 Apr 2021 09:41:54 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id n5sm2895537lfh.173.2021.04.08.09.41.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Apr 2021 09:41:53 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 7971B56015C; Thu, 8 Apr 2021 19:41:52 +0300 (MSK) To: tml Date: Thu, 8 Apr 2021 19:41:45 +0300 Message-Id: <20210408164151.1759348-1-gorcunov@gmail.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v21 0/6] box: implement box.lib Lua module X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Cc: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" v1-v3 are development ones and not sent. v5 (by vlad): - drop exists, list methods: they are redundant - rename cfunc to cbox - when create a function make it callable Lua object - initialize cbox out of modules - fix error in passing module name for reloading - make api been cbox.func.[create|drop] and cbox.module.reload - fix test for OSX sake v6 (by vlad): - move module handling into module_cache file. v7: - development v8: - use rbtree for function instance storage, since i don't like the idea of unexpected rehashing of values in case of massive number of functions allocated - use reference counter and free function instance if only load/unload are coupled - keep a pointer to the function inside Lua object so we don't need to lookup on every function call. this force us to implement __gc method - use new API and update docs v9: - development v10: - use hashes for function names lookup - simply function loads counting - use luaL_register_module and luaL_register_type for easier methods registering - carry functions as userdata object v11: - development v12: - switch to new API as been discussed in https://lists.tarantool.org/tarantool-patches/e186c454-6765-4776-6433-f3f791ff4c27@tarantool.org/ v13: - development v14: - switch to refs to carry module usage - drop func_name structure renaming - carry two hashes for backward compatibility with functions created via box.schema.func help - complete rework of cmod and most parts of module_cache - account for file statistics to invalidate module cache - new API for cmod, no more :reload, the :load procedure uses cache invalidation - update test cases - still there is no GC test since I didn't manage to deal with it v15: - report module state cached/orphan - update test cases - do not prevent functions lookup in orphan modules - there was an idea to use box.shema.func cache as on top of cmod's one, but this doesn't work because in case if module doesnt exist in any caches we would put it into into cmod's one as well but there wont be a module on cmod level which would clean it up later (which makes code a way more comple if we choose to track state of modules). v16: - internal v17: - drop idea of unifying box.schema.func and cmod functions cache, it brings more problems than solves due to too different context of execution; - make cmod self consistent, which shrink patch series size ~1/5 in compare with previous attempts; - improve tests to account internal states of modules and functions (tt_dev key in reports). v18: - implement pass-through cache for modules loading, for this sake 'struct module' uses cmod internally; - improve tests to cover sole cmod case and a mixture of box.schema.func and cmod to make sure the caches are not corrupted. v19: - move module handling into separate subsystem; - switch box.schema.func and cmod to use this shared code to eliminate code duplication; - update tests. v20: - fix potential nil dereference in schema_init while I'm in the code; - during working on the series found a bug in module recovery, fixed it in the series because my further work depends on it; - main module which allows to create C functions now named as box.lib; - structures renaming: - box.schema.func uses struct schema_module to carry underlied struct module; - box.lib uses struct module directly and struct box_module_func to carry cache of functions. - name unification: - schema_module variables are named as `mod`; - box.schema.func api prefixed as `schema_module_x`; - debug properties for module counting prefixed as `debug_x`; - the test remains disabled because - I run it locally all the time - better to have it in repo and enable then since patching test-run can take weeks, it is slow procedure v21: - rename functions and members from Vlad's comments - use @base name for lowlevel module and functions - use @module name for toplevel instances - rename __schema_module_load to schema_do_module_load - rename schema_func_c_load to func_c_load_from - rename box_lib.[ch] to plain lib.[ch] - cache free methods only clear the hash table itself - in cache_put make sure there is an empty slot we're touching - unify `const char *` usage - use lua_isstring instead of LUA_TSTRING test - optimize clearing of user data on Lua level via clear_udata helper - don't use tt_static hepers - in schema_module_reload restore failing function immediately and the proceed the remaining ones - move ERRINJ_DYN_MODULE_COUNT to module_cache so that old tests are passing - more detailed testing of module loading - make box.lib enabled by default even without box.cfg{} I pushed it and since github actions are not that fast I ran tests locally as well Statistics: * pass: 1250 * disabled: 70 issue https://github.com/tarantool/tarantool/issues/4642 branch gorcunov/gh-4642-func-ro-21 Cyrill Gorcunov (6): box/func: fix modules functions restore box/func: module_reload -- drop redundant argument box/module_cache: introduce modules subsystem box/schema.func: switch to new module api box: implement box.lib module test: add box.lib test changelogs/unreleased/add-box-lib.md | 4 + changelogs/unreleased/fix-module-reload.md | 8 + src/box/CMakeLists.txt | 2 + src/box/box.cc | 4 +- src/box/call.c | 9 +- src/box/func.c | 568 ++++++++----------- src/box/func.h | 28 +- src/box/func_def.h | 14 - src/box/lua/init.c | 2 + src/box/lua/lib.c | 606 +++++++++++++++++++++ src/box/lua/lib.h | 25 + src/box/lua/load_cfg.lua | 1 + src/box/module_cache.c | 491 +++++++++++++++++ src/box/module_cache.h | 205 +++++++ src/main.cc | 3 + test/box/CMakeLists.txt | 8 + test/box/cfunc1.c | 58 ++ test/box/cfunc2.c | 137 +++++ test/box/cfunc3.c | 25 + test/box/cfunc4.c | 28 + test/box/func_restore.result | 95 ++++ test/box/func_restore.test.lua | 50 ++ test/box/func_restore1.c | 33 ++ test/box/func_restore2.c | 27 + test/box/func_restore3.c | 27 + test/box/func_restore4.c | 27 + test/box/lib.result | 530 ++++++++++++++++++ test/box/lib.test.lua | 203 +++++++ test/box/misc.result | 1 + 29 files changed, 2847 insertions(+), 372 deletions(-) create mode 100644 changelogs/unreleased/add-box-lib.md create mode 100644 changelogs/unreleased/fix-module-reload.md create mode 100644 src/box/lua/lib.c create mode 100644 src/box/lua/lib.h create mode 100644 src/box/module_cache.c create mode 100644 src/box/module_cache.h create mode 100644 test/box/cfunc1.c create mode 100644 test/box/cfunc2.c create mode 100644 test/box/cfunc3.c create mode 100644 test/box/cfunc4.c create mode 100644 test/box/func_restore.result create mode 100644 test/box/func_restore.test.lua create mode 100644 test/box/func_restore1.c create mode 100644 test/box/func_restore2.c create mode 100644 test/box/func_restore3.c create mode 100644 test/box/func_restore4.c create mode 100644 test/box/lib.result create mode 100644 test/box/lib.test.lua base-commit: b88df4dc86c64029e26b3785761df209d0bfc84f -- 2.30.2 Cummulative diff from v20 --- diff --git a/changelogs/unreleased/add-box-lib.md b/changelogs/unreleased/add-box-lib.md new file mode 100644 index 000000000..3fe7a1b01 --- /dev/null +++ b/changelogs/unreleased/add-box-lib.md @@ -0,0 +1,4 @@ +## feature/core + +* Introduce `box.lib` module which allows to load and execute + C stored procedures on read-only nodes (gh-4642). diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md index 7e189617f..e44118aa8 100644 --- a/changelogs/unreleased/fix-module-reload.md +++ b/changelogs/unreleased/fix-module-reload.md @@ -1,4 +1,8 @@ ## bugfix/core -* Fix module reloading procedure which may crash in case if - new module is corrupted (gh-4642). +* Fix crash in case of reloading a compiled module when the + new module lacks some of functions which were present in the + former code. In turn this event triggers a fallback procedure + where we restore old functions but instead of restoring each + function we process a sole entry only leading to the crash + later when these restored functions are called (gh-5968). diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt index 95c65eb71..09f81afd8 100644 --- a/src/box/CMakeLists.txt +++ b/src/box/CMakeLists.txt @@ -196,7 +196,7 @@ add_library(box STATIC lua/call.c lua/cfg.cc lua/console.c - lua/box_lib.c + lua/lib.c lua/serialize_lua.c lua/tuple.c lua/slab.c diff --git a/src/box/func.c b/src/box/func.c index 54d0cbc68..4119d2eec 100644 --- a/src/box/func.c +++ b/src/box/func.c @@ -32,7 +32,6 @@ #include "fiber.h" #include "assoc.h" #include "lua/call.h" -#include "errinj.h" #include "diag.h" #include "port.h" #include "schema.h" @@ -55,7 +54,7 @@ struct func_name { */ struct schema_module { /** Low level module instance. */ - struct module *module; + struct module *base; /** List of imported functions. */ struct rlist funcs; /** Reference counter. */ @@ -77,14 +76,14 @@ struct func_c { /** * A schema module the function belongs to. */ - struct schema_module *mod; + struct schema_module *module; }; static void -schema_module_ref(struct schema_module *mod); +schema_module_ref(struct schema_module *module); static void -schema_module_unref(struct schema_module *mod); +schema_module_unref(struct schema_module *module); /*** * Split function name to symbol and package names. @@ -127,10 +126,10 @@ schema_module_free(void) { while (mh_size(modules) > 0) { mh_int_t i = mh_first(modules); - struct schema_module *mod = mh_strnptr_node(modules, i)->val; - schema_module_unref(mod); + mh_strnptr_del(modules, i, NULL); } mh_strnptr_delete(modules); + modules = NULL; } /** @@ -149,16 +148,16 @@ cache_find(const char *name, const char *name_end) * Save a module to the modules cache. */ static int -cache_put(struct schema_module *mod) +cache_put(struct schema_module *module) { - const char *str = mod->module->package; - size_t len = mod->module->package_len; + const char *str = module->base->package; + size_t len = module->base->package_len; const struct mh_strnptr_node_t strnode = { .str = str, .len = len, .hash = mh_strn_hash(str, len), - .val = mod, + .val = module, }; if (mh_strnptr_put(modules, &strnode, NULL, NULL) == mh_end(modules)) { @@ -172,27 +171,27 @@ cache_put(struct schema_module *mod) * Update a module in the modules cache. */ static void -cache_update(struct schema_module *mod) +cache_update(struct schema_module *module) { - const char *str = mod->module->package; - size_t len = mod->module->package_len; + const char *str = module->base->package; + size_t len = module->base->package_len; mh_int_t i = mh_strnptr_find_inp(modules, str, len); if (i == mh_end(modules)) panic("func: failed to update cache: %s", str); mh_strnptr_node(modules, i)->str = str; - mh_strnptr_node(modules, i)->val = mod; + mh_strnptr_node(modules, i)->val = module; } /** * Delete a module from the module cache. */ static void -cache_del(struct schema_module *mod) +cache_del(struct schema_module *module) { - const char *str = mod->module->package; - size_t len = mod->module->package_len; + const char *str = module->base->package; + size_t len = module->base->package_len; mh_int_t i = mh_strnptr_find_inp(modules, str, len); if (i != mh_end(modules)) { @@ -202,72 +201,65 @@ cache_del(struct schema_module *mod) * The module may be already reloaded so * the cache carries a new entry instead. */ - if (v == mod) + if (v == module) mh_strnptr_del(modules, i, NULL); } } /** Delete a module. */ static void -schema_module_delete(struct schema_module *mod) +schema_module_delete(struct schema_module *module) { - struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT); - if (e != NULL) - --e->iparam; - module_unload(mod->module); - TRASH(mod); - free(mod); + module_unload(module->base); + TRASH(module); + free(module); } /** Increment reference to a module. */ static void -schema_module_ref(struct schema_module *mod) +schema_module_ref(struct schema_module *module) { - assert(mod->refs >= 0); - ++mod->refs; + assert(module->refs >= 0); + ++module->refs; } /** Decrement reference to a module and delete it if last one. */ static void -schema_module_unref(struct schema_module *mod) +schema_module_unref(struct schema_module *module) { - assert(mod->refs > 0); - if (--mod->refs == 0) { - cache_del(mod); - schema_module_delete(mod); + assert(module->refs > 0); + if (--module->refs == 0) { + cache_del(module); + schema_module_delete(module); } } static struct schema_module * -__schema_module_load(const char *name, size_t len, bool force) +schema_do_module_load(const char *name, size_t len, bool force) { - struct schema_module *mod = malloc(sizeof(*mod)); - if (mod != NULL) { - mod->module = NULL; - mod->refs = 0; - rlist_create(&mod->funcs); + struct schema_module *module = malloc(sizeof(*module)); + if (module != NULL) { + module->base = NULL; + module->refs = 0; + rlist_create(&module->funcs); } else { - diag_set(OutOfMemory, sizeof(*mod), + diag_set(OutOfMemory, sizeof(*module), "malloc", "struct schema_module"); return NULL; } if (force) - mod->module = module_load_force(name, len); + module->base = module_load_force(name, len); else - mod->module = module_load(name, len); + module->base = module_load(name, len); - if (!mod->module) { - free(mod); + if (module->base == NULL) { + free(module); return NULL; } - struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT); - if (e != NULL) - ++e->iparam; - - schema_module_ref(mod); - return mod; + schema_module_ref(module); + return module; } /** @@ -276,7 +268,7 @@ __schema_module_load(const char *name, size_t len, bool force) static struct schema_module * schema_module_load(const char *name, const char *name_end) { - return __schema_module_load(name, name_end - name, false); + return schema_do_module_load(name, name_end - name, false); } /** @@ -285,7 +277,7 @@ schema_module_load(const char *name, const char *name_end) static struct schema_module * schema_module_load_force(const char *name, const char *name_end) { - return __schema_module_load(name, name_end - name, true); + return schema_do_module_load(name, name_end - name, true); } static struct func_vtab func_c_vtab; @@ -294,34 +286,33 @@ static struct func_vtab func_c_vtab; static void func_c_create(struct func_c *func_c) { - func_c->mod = NULL; + func_c->module = NULL; func_c->base.vtab = &func_c_vtab; rlist_create(&func_c->item); module_func_create(&func_c->mf); } static int -schema_func_c_load(struct schema_module *mod, const char *func_name, - struct func_c *func) +func_c_load_from(struct func_c *func, struct schema_module *module, + const char *func_name) { assert(module_func_is_empty(&func->mf)); - if (module_func_load(mod->module, func_name, &func->mf) != 0) + if (module_func_load(module->base, func_name, &func->mf) != 0) return -1; - func->mod = mod; - rlist_move(&mod->funcs, &func->item); - schema_module_ref(mod); - + func->module = module; + rlist_move(&module->funcs, &func->item); + schema_module_ref(module); return 0; } static void -schema_func_c_unload(struct func_c *func) +func_c_unload(struct func_c *func) { if (!module_func_is_empty(&func->mf)) { rlist_del(&func->item); - schema_module_unref(func->mod); + schema_module_unref(func->module); module_func_unload(&func->mf); func_c_create(func); } @@ -353,14 +344,16 @@ schema_module_reload(const char *package, const char *package_end) rlist_foreach_entry_safe(func, &old->funcs, item, tmp_func) { struct func_name name; func_split_name(func->base.def->name, &name); - schema_func_c_unload(func); - if (schema_func_c_load(new, name.sym, func) != 0) { + func_c_unload(func); + if (func_c_load_from(func, new, name.sym) != 0) { /* - * WARN: A hack accessing new->funcs directly - * to start restore from this failing function. + * We can restore failing function immediately + * and then all previously migrated ones. */ - rlist_move(&new->funcs, &func->item); - goto restore; + if (func_c_load_from(func, old, name.sym) != 0) + goto restore_fail; + else + goto restore; } } cache_update(new); @@ -369,25 +362,28 @@ schema_module_reload(const char *package, const char *package_end) return 0; restore: /* - * Some old functions are not found int the new module, - * thus restore all migrated functions back to original. + * Some old functions are not found in the new module, + * thus restore all migrated functions back to the original. */ rlist_foreach_entry_safe(func, &new->funcs, item, tmp_func) { struct func_name name; func_split_name(func->base.def->name, &name); - schema_func_c_unload(func); - if (schema_func_c_load(old, name.sym, func) != 0) { - /* - * Something strange was happen, an early loaden - * function was not found in an old dso. - */ - panic("Can't restore module function, " - "server state is inconsistent"); - } + func_c_unload(func); + if (func_c_load_from(func, old, name.sym) != 0) + goto restore_fail; } schema_module_unref(old); schema_module_unref(new); return -1; + +restore_fail: + /* + * Something strange was happen, an early loaden + * function was not found in an old dso. + */ + panic("Can't restore module function, " + "server state is inconsistent"); + return -1; } static struct func * @@ -454,7 +450,7 @@ func_c_destroy(struct func *base) assert(base->vtab == &func_c_vtab); assert(base != NULL && base->def->language == FUNC_LANGUAGE_C); struct func_c *func = (struct func_c *) base; - schema_func_c_unload(func); + func_c_unload(func); TRASH(base); free(func); } @@ -469,28 +465,28 @@ func_c_load(struct func_c *func) struct func_name name; func_split_name(func->base.def->name, &name); - struct schema_module *cached, *mod; + struct schema_module *cached, *module; cached = cache_find(name.package, name.package_end); if (cached == NULL) { - mod = schema_module_load(name.package, name.package_end); - if (mod == NULL) + module = schema_module_load(name.package, name.package_end); + if (module == NULL) return -1; - if (cache_put(mod)) { - schema_module_unref(mod); + if (cache_put(module)) { + schema_module_unref(module); return -1; } } else { - mod = cached; - schema_module_ref(mod); + module = cached; + schema_module_ref(module); } - int rc = schema_func_c_load(mod, name.sym, func); + int rc = func_c_load_from(func, module, name.sym); /* * There is no explicit module loading in this * interface so each function carries a reference * by their own. */ - schema_module_unref(mod); + schema_module_unref(module); return rc; } diff --git a/src/box/lua/init.c b/src/box/lua/init.c index 3fb433e7a..3a6d60864 100644 --- a/src/box/lua/init.c +++ b/src/box/lua/init.c @@ -60,7 +60,7 @@ #include "box/lua/cfg.h" #include "box/lua/xlog.h" #include "box/lua/console.h" -#include "box/lua/box_lib.h" +#include "box/lua/lib.h" #include "box/lua/tuple.h" #include "box/lua/execute.h" #include "box/lua/key_def.h" diff --git a/src/box/lua/box_lib.c b/src/box/lua/lib.c similarity index 87% rename from src/box/lua/box_lib.c rename to src/box/lua/lib.c index ce2ef8902..78aee37b5 100644 --- a/src/box/lua/box_lib.c +++ b/src/box/lua/lib.c @@ -11,10 +11,8 @@ #include "box/error.h" #include "box/port.h" -#include "tt_static.h" - #include "assoc.h" -#include "box_lib.h" +#include "lib.h" #include "diag.h" #include "module_cache.h" @@ -25,7 +23,7 @@ */ struct box_module_func { /** C function to call. */ - struct module_func mf; + struct module_func base; /** Number of references. */ int64_t refs; /** Length of functon name in @a key. */ @@ -53,13 +51,20 @@ get_udata(struct lua_State *L, const char *uname) return pptr != NULL ? *pptr : NULL; } -/** Set data to a new value. */ -static void -set_udata(struct lua_State *L, const char *uname, void *ptr) +/** + * Get pointer associated with an object and clear it + * returning previously associated data. + */ +static void * +clear_udata(struct lua_State *L, const char *uname) { void **pptr = luaL_testudata(L, 1, uname); - assert(pptr != NULL); - *pptr = ptr; + if (pptr != NULL) { + void *ptr = *pptr; + *pptr = NULL; + return ptr; + } + return NULL; } /** Setup a new data and associate it with an object. */ @@ -129,7 +134,7 @@ cache_del(struct box_module_func *cf) static int lbox_module_load(struct lua_State *L) { - const char msg_noname[] = "Expects box.lib.load(\'name\') " + const char *msg_noname = "Expects box.lib.load(\'name\') " "but no name passed"; if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) { @@ -173,14 +178,13 @@ lbox_module_unload(struct lua_State *L) return luaT_error(L); } - struct module *m = get_udata(L, uname_lib); + struct module *m = clear_udata(L, uname_lib); if (m == NULL) { diag_set(IllegalParams, "The module is unloaded"); return luaT_error(L); } - - set_udata(L, uname_lib, NULL); module_unload(m); + lua_pushboolean(L, true); return 1; } @@ -189,6 +193,9 @@ lbox_module_unload(struct lua_State *L) static int lbox_module_index(struct lua_State *L) { + /* + * Process metamethods such as "module:load" first. + */ lua_getmetatable(L, 1); lua_pushvalue(L, 2); lua_rawget(L, -2); @@ -202,7 +209,7 @@ lbox_module_index(struct lua_State *L) } const char *key = lua_tostring(L, 2); - if (key == NULL || lua_type(L, 2) != LUA_TSTRING) { + if (key == NULL || !lua_isstring(L, 2)) { diag_set(IllegalParams, "Bad params, use __index()"); return luaT_error(L); @@ -220,7 +227,8 @@ lbox_module_index(struct lua_State *L) lua_pushnumber(L, m->refs); return 1; } else if (strcmp(key, "debug_ptr") == 0) { - const char *s = tt_sprintf("%p", m); + char s[64]; + snprintf(s, sizeof(s), "%p", m); lua_pushstring(L, s); return 1; } @@ -247,11 +255,9 @@ lbox_module_serialize(struct lua_State *L) static int lbox_module_gc(struct lua_State *L) { - struct module *m = get_udata(L, uname_lib); - if (m != NULL) { - set_udata(L, uname_lib, NULL); + struct module *m = clear_udata(L, uname_lib); + if (m != NULL) module_unload(m); - } return 0; } @@ -267,7 +273,7 @@ box_module_func_ref(struct box_module_func *cf) static void box_module_func_delete(struct box_module_func *cf) { - assert(module_func_is_empty(&cf->mf)); + assert(module_func_is_empty(&cf->base)); TRASH(cf); free(cf); } @@ -278,7 +284,7 @@ box_module_func_unref(struct box_module_func *cf) { assert(cf->refs > 0); if (--cf->refs == 0) { - module_func_unload(&cf->mf); + module_func_unload(&cf->base); cache_del(cf); box_module_func_delete(cf); } @@ -315,17 +321,17 @@ box_module_func_new(struct module *m, const char *key, size_t len, size_t sym_le cf->sym_len = sym_len; cf->refs = 0; - module_func_create(&cf->mf); + module_func_create(&cf->base); memcpy(cf->key, key, len); cf->key[len] = '\0'; - if (module_func_load(m, box_module_func_name(cf), &cf->mf) != 0) { + if (module_func_load(m, box_module_func_name(cf), &cf->base) != 0) { box_module_func_delete(cf); return NULL; } if (cache_put(cf) != 0) { - module_func_unload(&cf->mf); + module_func_unload(&cf->base); box_module_func_delete(cf); return NULL; } @@ -360,7 +366,7 @@ 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"; + const char *fmt_noname = "Expects %s(\'name\') but no name passed"; if (lua_gettop(L) != 2 || !lua_isstring(L, 2)) { diag_set(IllegalParams, fmt_noname, method); @@ -377,10 +383,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; if (sym_len < 1) { diag_set(IllegalParams, fmt_noname, method); return luaT_error(L); + } else if (sym_len > max_sym_len) { + diag_set(IllegalParams, "Symbol \'%s\' is too long (max %zd)", + sym, max_sym_len); + return luaT_error(L); } /* @@ -388,9 +399,14 @@ lbox_module_load_func(struct lua_State *L) * since the hash is global it should be unique * 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. */ - const char *key = tt_sprintf("%p.%s.%s", (void *)m, - m->package, sym); + char key[PATH_MAX + 2 * max_sym_len]; + snprintf(key, sizeof(key), "%p.%s.%s", + (void *)m, m->package, sym); size_t len = strlen(key); struct box_module_func *cf = cache_find(key, len); @@ -426,13 +442,11 @@ lbox_func_unload(struct lua_State *L) return luaT_error(L); } - struct box_module_func *cf = get_udata(L, uname_func); + struct box_module_func *cf = clear_udata(L, uname_func); if (cf == NULL) { diag_set(IllegalParams, "The function is unloaded"); return luaT_error(L); } - - set_udata(L, uname_func, NULL); box_module_func_unref(cf); lua_pushboolean(L, true); @@ -443,6 +457,9 @@ lbox_func_unload(struct lua_State *L) static int lbox_func_index(struct lua_State *L) { + /* + * Process metamethods such as "func:unload" first. + */ lua_getmetatable(L, 1); lua_pushvalue(L, 2); lua_rawget(L, -2); @@ -456,7 +473,7 @@ lbox_func_index(struct lua_State *L) } const char *key = lua_tostring(L, 2); - if (key == NULL || lua_type(L, 2) != LUA_TSTRING) { + if (key == NULL || !lua_isstring(L, 2)) { diag_set(IllegalParams, "Bad params, use __index()"); return luaT_error(L); @@ -477,11 +494,12 @@ lbox_func_index(struct lua_State *L) lua_pushstring(L, cf->key); return 1; } else if (strcmp(key, "debug_module_ptr") == 0) { - const char *s = tt_sprintf("%p", cf->mf.module); + char s[64]; + snprintf(s, sizeof(s), "%p", cf->base.module); lua_pushstring(L, s); return 1; } else if (strcmp(key, "debug_module_refs") == 0) { - lua_pushnumber(L, cf->mf.module->refs); + lua_pushnumber(L, cf->base.module->refs); return 1; } return 0; @@ -507,11 +525,9 @@ lbox_func_serialize(struct lua_State *L) static int lbox_func_gc(struct lua_State *L) { - struct box_module_func *cf = get_udata(L, uname_func); - if (cf != NULL) { - set_udata(L, uname_func, NULL); + struct box_module_func *cf = clear_udata(L, uname_func); + if (cf != NULL) box_module_func_unref(cf); - } return 0; } @@ -539,7 +555,7 @@ lbox_func_call(struct lua_State *L) struct port ret; - if (module_func_call(&cf->mf, &args, &ret) != 0) { + if (module_func_call(&cf->base, &args, &ret) != 0) { port_destroy(&args); return luaT_error(L); } diff --git a/src/box/lua/box_lib.h b/src/box/lua/lib.h similarity index 100% rename from src/box/lua/box_lib.h rename to src/box/lua/lib.h diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua index 44bb95ed1..374524973 100644 --- a/src/box/lua/load_cfg.lua +++ b/src/box/lua/load_cfg.lua @@ -597,6 +597,7 @@ local box_cfg_guard_whitelist = { error = true; internal = true; index = true; + lib = true; session = true; tuple = true; runtime = true; diff --git a/src/box/module_cache.c b/src/box/module_cache.c index 2cd2f2e8b..3dcb40566 100644 --- a/src/box/module_cache.c +++ b/src/box/module_cache.c @@ -10,9 +10,13 @@ #include #include +#include +#include + #include "assoc.h" #include "diag.h" #include "fiber.h" +#include "errinj.h" #include "module_cache.h" #include "box/error.h" @@ -26,7 +30,7 @@ static struct mh_strnptr_t *module_cache = NULL; /** * Helpers for cache manipulations. */ -static void * +static struct module * cache_find(const char *str, size_t len) { mh_int_t e = mh_strnptr_find_inp(module_cache, str, len); @@ -59,12 +63,21 @@ cache_put(struct module *m) .val = m, }; - mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL); + struct mh_strnptr_node_t prev; + struct mh_strnptr_node_t *prev_ptr = &prev; + + mh_int_t e = mh_strnptr_put(module_cache, &nd, &prev_ptr, NULL); if (e == mh_end(module_cache)) { diag_set(OutOfMemory, sizeof(nd), "malloc", "module_cache node"); return -1; } + + /* + * Just to make sure we haven't replaced something, the + * entries must be explicitly deleted. + */ + assert(prev_ptr == NULL); return 0; } @@ -160,6 +173,9 @@ module_unref(struct module *m) { assert(m->refs > 0); if (--m->refs == 0) { + struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT); + if (e != NULL) + --e->iparam; cache_del(m); dlclose(m->handle); TRASH(m); @@ -359,6 +375,10 @@ module_new(const char *package, size_t package_len, goto error; } + struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT); + if (e != NULL) + ++e->iparam; + module_ref(m); return m; @@ -450,13 +470,10 @@ module_unload(struct module *m) void module_free(void) { - mh_int_t e; - - mh_foreach(module_cache, e) { - struct module *m = mh_strnptr_node(module_cache, e)->val; - module_unload(m); + while (mh_size(module_cache) > 0) { + mh_int_t e = mh_first(module_cache); + mh_strnptr_del(module_cache, e, NULL); } - mh_strnptr_delete(module_cache); module_cache = NULL; } diff --git a/src/box/module_cache.h b/src/box/module_cache.h index 18eb3866a..24fdf8d85 100644 --- a/src/box/module_cache.h +++ b/src/box/module_cache.h @@ -8,9 +8,6 @@ #include -#include -#include - #include "trivia/config.h" #if defined(__cplusplus) diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt index c9dcbbb7b..51db15653 100644 --- a/test/box/CMakeLists.txt +++ b/test/box/CMakeLists.txt @@ -4,6 +4,8 @@ build_module(reload1 reload1.c) build_module(reload2 reload2.c) build_module(func_restore1 func_restore1.c) build_module(func_restore2 func_restore2.c) +build_module(func_restore3 func_restore3.c) +build_module(func_restore4 func_restore4.c) build_module(tuple_bench tuple_bench.c) build_module(cfunc1 cfunc1.c) build_module(cfunc2 cfunc2.c) diff --git a/test/box/func_restore.result b/test/box/func_restore.result index e679f4eb8..7cd9e67c4 100644 --- a/test/box/func_restore.result +++ b/test/box/func_restore.result @@ -1,7 +1,7 @@ -- test-run result file version 2 -- --- Test that function can be restored to the --- former values when new module can't be +-- Test that compiled C function can be restored +-- to the former values when new module can't be -- loaded for some reason (say there some -- missing functions). -- @@ -26,9 +26,6 @@ path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext | --- | ... -path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext - | --- - | ... _ = pcall(fio.unlink(path_func_restore)) | --- @@ -58,8 +55,6 @@ box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1') | --- | ... --- Order *does* matter since we bind functions in --- a list where entries are added to the top. box.func['func_restore.echo_3']:call() | --- | - 3 @@ -73,31 +68,28 @@ box.func['func_restore.echo_1']:call() | - 1 | ... -_ = pcall(fio.unlink(path_func_restore)) - | --- - | ... -fio.symlink(path_func_bad, path_func_restore) +function run_restore(path) \ + _ = pcall(fio.unlink(path_func_restore)) \ + fio.symlink(path, path_func_restore) \ + \ + local ok, _ = pcall(box.schema.func.reload, "func_restore") \ + assert(not ok) \ + \ + box.func['func_restore.echo_1']:call() \ + box.func['func_restore.echo_2']:call() \ + box.func['func_restore.echo_3']:call() \ +end | --- - | - true | ... -ok, _ = pcall(box.schema.func.reload, "func_restore") +bad_modules = { \ + fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \ +} | --- | ... -assert(not ok) - | --- - | - true - | ... -box.func['func_restore.echo_1']:call() +for k, v in ipairs(bad_modules) do run_restore(v) end | --- - | - 1 - | ... -box.func['func_restore.echo_2']:call() - | --- - | - 2 - | ... -box.func['func_restore.echo_3']:call() - | --- - | - 3 | ... diff --git a/test/box/func_restore.test.lua b/test/box/func_restore.test.lua index 8a38a6074..4bd05769d 100644 --- a/test/box/func_restore.test.lua +++ b/test/box/func_restore.test.lua @@ -1,6 +1,6 @@ -- --- Test that function can be restored to the --- former values when new module can't be +-- Test that compiled C function can be restored +-- to the former values when new module can't be -- loaded for some reason (say there some -- missing functions). -- @@ -13,7 +13,6 @@ ext = (jit.os == "OSX" and "dylib" or "so") path_func_restore = fio.pathjoin(build_path, "test/box/func_restore.") .. ext path_func_good = fio.pathjoin(build_path, "test/box/func_restore1.") .. ext -path_func_bad = fio.pathjoin(build_path, "test/box/func_restore2.") .. ext _ = pcall(fio.unlink(path_func_restore)) fio.symlink(path_func_good, path_func_restore) @@ -26,18 +25,26 @@ box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_3') box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_2') box.schema.user.grant('guest', 'execute', 'function', 'func_restore.echo_1') --- Order *does* matter since we bind functions in --- a list where entries are added to the top. box.func['func_restore.echo_3']:call() box.func['func_restore.echo_2']:call() box.func['func_restore.echo_1']:call() -_ = pcall(fio.unlink(path_func_restore)) -fio.symlink(path_func_bad, path_func_restore) - -ok, _ = pcall(box.schema.func.reload, "func_restore") -assert(not ok) - -box.func['func_restore.echo_1']:call() -box.func['func_restore.echo_2']:call() -box.func['func_restore.echo_3']:call() +function run_restore(path) \ + _ = pcall(fio.unlink(path_func_restore)) \ + fio.symlink(path, path_func_restore) \ + \ + local ok, _ = pcall(box.schema.func.reload, "func_restore") \ + assert(not ok) \ + \ + box.func['func_restore.echo_1']:call() \ + box.func['func_restore.echo_2']:call() \ + box.func['func_restore.echo_3']:call() \ +end + +bad_modules = { \ + fio.pathjoin(build_path, "test/box/func_restore2.") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore3.") .. ext, \ + fio.pathjoin(build_path, "test/box/func_restore4.") .. ext, \ +} + +for k, v in ipairs(bad_modules) do run_restore(v) end diff --git a/test/box/func_restore1.c b/test/box/func_restore1.c index ffd69fd2b..891ec0136 100644 --- a/test/box/func_restore1.c +++ b/test/box/func_restore1.c @@ -14,7 +14,6 @@ echo_num(box_function_ctx_t *ctx, const char *args, return 0; } - int echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) { diff --git a/test/box/func_restore2.c b/test/box/func_restore2.c index 0d77e78b2..d9d6de8df 100644 --- a/test/box/func_restore2.c +++ b/test/box/func_restore2.c @@ -14,7 +14,6 @@ echo_num(box_function_ctx_t *ctx, const char *args, return 0; } - int echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) { diff --git a/test/box/func_restore3.c b/test/box/func_restore3.c new file mode 100644 index 000000000..e38b44400 --- /dev/null +++ b/test/box/func_restore3.c @@ -0,0 +1,27 @@ +#include +#include +#include + +#include "module.h" + +static int +echo_num(box_function_ctx_t *ctx, const char *args, + const char *args_end, unsigned int num) +{ + char res[16]; + char *end = mp_encode_uint(res, num); + box_return_mp(ctx, res, end); + return 0; +} + +int +echo_2(box_function_ctx_t *ctx, const char *args, const char *args_end) +{ + return echo_num(ctx, args, args_end, 2); +} + +int +echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end) +{ + return echo_num(ctx, args, args_end, 3); +} diff --git a/test/box/func_restore4.c b/test/box/func_restore4.c new file mode 100644 index 000000000..4e97ff0a0 --- /dev/null +++ b/test/box/func_restore4.c @@ -0,0 +1,27 @@ +#include +#include +#include + +#include "module.h" + +static int +echo_num(box_function_ctx_t *ctx, const char *args, + const char *args_end, unsigned int num) +{ + char res[16]; + char *end = mp_encode_uint(res, num); + box_return_mp(ctx, res, end); + return 0; +} + +int +echo_1(box_function_ctx_t *ctx, const char *args, const char *args_end) +{ + return echo_num(ctx, args, args_end, 1); +} + +int +echo_3(box_function_ctx_t *ctx, const char *args, const char *args_end) +{ + return echo_num(ctx, args, args_end, 3); +}