From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DACE1469719 for ; Fri, 30 Oct 2020 01:15:54 +0300 (MSK) From: Vladislav Shpilevoy References: <20201014133535.224573-1-gorcunov@gmail.com> <20201014133535.224573-2-gorcunov@gmail.com> Message-ID: <8706dd6e-0b8d-f3f1-2344-f1b24906eecd@tarantool.org> Date: Thu, 29 Oct 2020 23:15:51 +0100 MIME-Version: 1.0 In-Reply-To: <20201014133535.224573-2-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v8 1/4] box/func: factor out c function entry structure List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Hi! Thanks for the patch! See 3 comments below. On 14.10.2020 15:35, Cyrill Gorcunov wrote: > Currently func_c structure is a wrapper over struct func > which in turn handles function definition, credentials > and etc, mostly to handle internal "_func" space. > > Such tight bound doesn't allow to reuse func_c in any > other context. But we're going to support C function > execution in read-only instances and for this we better > reuse already existing structures as much as possible > instead of breeding new ones. > > Thus lets extract module symbols into module_sym structure, > this allows us to reuse module path cache in other code. > > While extracting the structure rename "func" member to > "addr" since this exactly what the member represents: > an address of symbol in memory. > > Same time to be completely independent of "_func" specific lets > carry symbolic name inplace (ie member "name" is kind of redundant > when module_sym is used for "_func" handling where we can retrieve > the name via function definition but such definition won't be > available for non-persistent C functions which we will support > in next patches). > > The new structure allows us to load/unload general symbols via > module_sym_load() and module_sym_unload() helpers. > > Part-of #4642 > > Signed-off-by: Cyrill Gorcunov > --- > src/box/func.c | 156 +++++++++++++++++++++++-------------------------- > src/box/func.h | 43 ++++++++++++++ > 2 files changed, 116 insertions(+), 83 deletions(-) > > diff --git a/src/box/func.c b/src/box/func.c > index 8087c953f..75d2abb5f 100644 > --- a/src/box/func.c > +++ b/src/box/func.c > @@ -371,6 +361,53 @@ module_sym(struct module *module, const char *name) > return f; > } > > +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 > + 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. > + mod_sym->module = module; > + rlist_add(&module->funcs, &mod_sym->item); > + return 0; > +} > diff --git a/src/box/func.h b/src/box/func.h > index 581e468cb..acf2df9a9 100644 > --- a/src/box/func.h > +++ b/src/box/func.h > @@ -58,6 +58,29 @@ struct module { > char package[0]; > }; > > +/** > + * 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. > + struct module *module; > + /** > + * Function name definition. > + */ > + char *name; > +};