From: Cyrill Gorcunov <gorcunov@gmail.com> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v8 1/4] box/func: factor out c function entry structure Date: Fri, 30 Oct 2020 12:51:57 +0300 [thread overview] Message-ID: <20201030095157.GD198833@grain> (raw) In-Reply-To: <8706dd6e-0b8d-f3f1-2344-f1b24906eecd@tarantool.org> On Thu, Oct 29, 2020 at 11:15:51PM +0100, Vladislav Shpilevoy wrote: ... > > +int > > +module_sym_load(struct module_sym *mod_sym) > > +{ > > + assert(mod_sym->addr == NULL); > > + > > + struct func_name name; > > + func_split_name(mod_sym->name, &name); > > + > > + struct module *module = module_cache_find(name.package, > > + name.package_end); > > + if (module == NULL) { > > + /* Try to find loaded module in the cache */ > > + module = module_load(name.package, name.package_end); > > + if (module == NULL) > > + return -1; > > + if (module_cache_put(module)) { > > 1. Please, use explicit != 0. > https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style OK > > > + module_delete(module); > > + return -1; > > + } > > + } > > + > > + mod_sym->addr = module_sym(module, name.sym); > > + if (mod_sym->addr == NULL) > > + return -1; > > 2. If the module was loaded first time here, it is not unloaded in case of > an error in this place. Just like it was before the patch. The patch simply factor outs the old code. It doesn't improve it (because, lets be honest this is a min problem for module management -- we've to check for module symbol not at moment of calling it but rather at the moment when we load a function). That said the issue with module management is known and I think we need to rework modules code more deeply, but not in this series. In the series it remains exactly as it was. > > > > +/** > > + * Callable symbol bound to a module. > > + */ > > +struct module_sym { > > + /** > > + * Anchor for module membership. > > + */ > > + struct rlist item; > > + /** > > + * For C functions, address of the function. > > + */ > > + box_function_f addr; > > + /** > > + * Each stored function keeps a handle to the > > + * dynamic library for the C callback. > > + */ > > 3. Can't parse the comment. What is the 'C callback'? > And why is this function stored? After you extracted it > from struct func_c, it is not related to _func space, and > is not stored. It is stored in memory. The C callback is the function we call, so we keep a pointer to a module. If shuch comment confuses lets change it to something like "Each function keeps a handle to the module where the symbol relies"?
next prev parent reply other threads:[~2020-10-30 9:52 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-14 13:35 [Tarantool-patches] [PATCH v8 0/4] box/cbox: implement cfunc Lua module Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 1/4] box/func: factor out c function entry structure Cyrill Gorcunov 2020-10-29 22:15 ` Vladislav Shpilevoy 2020-10-30 9:51 ` Cyrill Gorcunov [this message] 2020-10-31 0:13 ` Vladislav Shpilevoy 2020-10-31 15:27 ` Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 2/4] module_cache: move module handling into own subsystem Cyrill Gorcunov 2020-10-29 22:15 ` Vladislav Shpilevoy 2020-10-30 10:15 ` Cyrill Gorcunov 2020-10-31 0:15 ` Vladislav Shpilevoy 2020-10-31 15:29 ` Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 3/4] box/cbox: implement cbox Lua module Cyrill Gorcunov 2020-10-29 22:15 ` Vladislav Shpilevoy 2020-10-30 12:51 ` Cyrill Gorcunov 2020-10-31 0:21 ` Vladislav Shpilevoy 2020-10-31 21:59 ` Cyrill Gorcunov 2020-11-01 8:26 ` Cyrill Gorcunov 2020-11-02 22:25 ` Vladislav Shpilevoy 2020-11-03 7:26 ` Cyrill Gorcunov 2020-11-12 22:54 ` Vladislav Shpilevoy 2020-11-13 18:30 ` Cyrill Gorcunov 2020-10-14 13:35 ` [Tarantool-patches] [PATCH v8 4/4] test: box/cfunc -- add simple module test Cyrill Gorcunov
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=20201030095157.GD198833@grain \ --to=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v8 1/4] box/func: factor out c function entry structure' \ /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