From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 18 Jun 2019 16:23:20 +0300 From: Vladimir Davydov Subject: Re: [PATCH v3 3/6] box: rework func object as a function frontend Message-ID: <20190618132320.r2vtyk2hprogwd6s@esperanza> References: <536bcc9ce5085679877fa91cea36f021168ffc2b.1560433806.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <536bcc9ce5085679877fa91cea36f021168ffc2b.1560433806.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Thu, Jun 13, 2019 at 05:08:23PM +0300, Kirill Shcherbatov wrote: > The function func object used to provide a call method only for > C functions. In scope of this patch it reworked to be a uniform > function call frontend both for C and Lua functions. > > Introduced classes func_c and func_lua, that provide own > constructors which produce implementation-specific object with > call and destroy methods. > > Needed for #4182, #1260 > --- > src/box/alter.cc | 1 + > src/box/call.c | 105 ++--------------------------- > src/box/func.c | 165 ++++++++++++++++++++++++++++++++++++++++----- > src/box/func.h | 25 ++++--- > src/box/func_def.h | 2 + > src/box/lua/call.c | 46 +++++++++++++ > src/box/lua/call.h | 5 ++ > src/box/session.cc | 11 ++- > src/box/session.h | 8 +++ > 9 files changed, 235 insertions(+), 133 deletions(-) Generally, the patch is fine. See some minor comments inline. > diff --git a/src/box/func.c b/src/box/func.c > index 740fad3d7..20ff56668 100644 > --- a/src/box/func.c > +++ b/src/box/func.c > @@ -381,19 +409,39 @@ func_new(struct func_def *def) > * checks (see user_has_data()). > */ > func->owner_credentials.auth_token = BOX_USER_MAX; /* invalid value */ > + return func; > +} > + > +static struct func_vtab func_c_vtab; > + > +struct func * > +func_c_new(struct func_def *def) Nit: should be static. > +{ > + (void) def; > + assert(def->language == FUNC_LANGUAGE_C); > + struct func_c *func = (struct func_c *) malloc(sizeof(struct func_c)); > + if (func == NULL) { > + diag_set(OutOfMemory, sizeof(*func), "malloc", "func"); > + return NULL; > + } > + func->base.vtab = &func_c_vtab; > func->func = NULL; > func->module = NULL; > - return func; > + return &func->base; > } > > static void > -func_unload(struct func *func) > +func_c_unload(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; > + > if (func->module) { > rlist_del(&func->item); > if (rlist_empty(&func->module->funcs)) { > struct func_name name; > - func_split_name(func->def->name, &name); > + func_split_name(func->base.def->name, &name); > module_cache_del(name.package, name.package_end); > } > module_gc(func->module); > @@ -407,12 +455,15 @@ func_unload(struct func *func) > * symbol from it). > */ > static int > -func_load(struct func *func) > +func_c_load(struct func *base) Why does it take struct func? Why not struct func_c? > { > + assert(base->vtab == &func_c_vtab); > + assert(base != NULL && base->def->language == FUNC_LANGUAGE_C); > + struct func_c *func = (struct func_c *) base; > assert(func->func == NULL); > > struct func_name name; > - func_split_name(func->def->name, &name); > + func_split_name(func->base.def->name, &name); > > struct module *module = module_cache_find(name.package, > name.package_end); > @@ -472,10 +526,87 @@ func_call(struct func *func, struct port *in_port, struct port *out_port) > return rc; > } > > +static struct func_vtab func_c_vtab = { > + .call = func_c_call, > + .destroy = func_c_unload, This looks confusing. Let's call the destructor 'func_c_destroy'. Also, I think that free(func) should be called not by the generic func_destroy(), but by the virtual destructor, because it's good to have malloc and free in the same compilation unit. I like func_c_load/func_c_unload symmetry. Let's make those functions take struct func_c instead of struct func. Then func_c_destroy would call func_c_unload and free struct func. > +}; > + > void > func_delete(struct func *func) > { > - func_unload(func); > + func->vtab->destroy(func); > free(func->def); > free(func); > } > + > +/** Check "EXECUTE" permissions for a given function. */ > +static int > +func_access_check(struct func *base) Nit: since you don't cast it to a subclass, better name it simply 'func', not 'base' IMO. > +{ > + struct credentials *credentials = effective_user(); > + /* > + * If the user has universal access, don't bother with > + * checks. No special check for ADMIN user is necessary > + * since ADMIN has universal access. > + */ > + if ((credentials->universal_access & (PRIV_X | PRIV_U)) == > + (PRIV_X | PRIV_U)) > + return 0; > + user_access_t access = PRIV_X | PRIV_U; > + /* Check access for all functions. */ > + access &= ~entity_access_get(SC_FUNCTION)[ > + credentials->auth_token].effective; > + user_access_t func_access = access & ~credentials->universal_access; > + if (/* Check for missing Usage access, ignore owner rights. */ > + func_access & PRIV_U || > + /* Check for missing specific access, respect owner rights. */ > + (base->def->uid != credentials->uid && > + func_access & ~base->access[credentials->auth_token].effective)) { Nit: bad indentation: func_access... should be aligned by the parenthesis. > diff --git a/src/box/func.h b/src/box/func.h > index d38a2b283..1271bde67 100644 > --- a/src/box/func.h > +++ b/src/box/func.h > @@ -62,24 +63,22 @@ struct module { > bool is_unloading; > }; > > +/** Virtual method table for func object. */ > +struct func_vtab { > + /** Call function with given arguments. */ > + int (*call)(struct func *func, struct port *in_port, > + struct port *out_port); > + /** Release implementation-specific function context. */ > + void (*destroy)(struct func *func); > +}; > + > /** > * Stored function. > */ > struct func { > struct func_def *def; > - /** > - * Anchor for module membership. > - */ > - struct rlist item; > - /** > - * For C functions, the body of the function. > - */ > - box_function_f func; > - /** > - * Each stored function keeps a handle to the > - * dynamic library for the C callback. > - */ > - struct module *module; > + /** Virtual method table. */ > + struct func_vtab *vtab; Nit: should be const. > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index e32845095..8d0328ef7 100644 > --- a/src/box/lua/call.c > +++ b/src/box/lua/call.c > @@ -523,6 +525,50 @@ box_lua_eval(const char *expr, uint32_t expr_len, > return box_process_lua(execute_lua_eval, &ctx, in_port, out_port); > } > > +struct func_lua { > + /** Function object base class. */ > + struct func base; > +}; > + > +static struct func_vtab func_lua_vtab; > + > +struct func * > +func_lua_new(struct func_def *def) > +{ > + (void) def; > + assert(def->language == FUNC_LANGUAGE_LUA); > + struct func_lua *func = > + (struct func_lua *) malloc(sizeof(struct func_lua)); > + if (func == NULL) { > + diag_set(OutOfMemory, sizeof(*func), "malloc", "func"); > + return NULL; > + } > + func->base.vtab = &func_lua_vtab; > + return &func->base; > +} > + > +static void > +func_lua_unload(struct func *base) Again, I think this should be called func_lua_destroy and this function should free func_lua struct. > +{ > + assert(base != NULL && base->def->language == FUNC_LANGUAGE_LUA); > + assert(base->vtab == &func_lua_vtab); > + (void) base; > +} > + > +static inline int > +func_lua_call(struct func *func, struct port *in_port, struct port *out_port) > +{ > + assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA); > + assert(func->vtab == &func_lua_vtab); > + return box_lua_call(func->def->name, func->def->name_len, > + in_port, out_port); > +} > + > +static struct func_vtab func_lua_vtab = { > + .call = func_lua_call, > + .destroy = func_lua_unload, > +}; > + > static int > lbox_module_reload(lua_State *L) > { > diff --git a/src/box/session.h b/src/box/session.h > index 3a7397146..6bafb8561 100644 > --- a/src/box/session.h > +++ b/src/box/session.h > @@ -312,6 +312,14 @@ access_check_session(struct user *user); > int > access_check_universe(user_access_t access); > > +/** > + * Check whether or not the current user can be granted > + * the requested access to the universe object. The comment is kinda confusing. Let's just say something like Same as access_check_universe(), but in case the current user doesn't have universal access, set AccessDeniedError for the given object type and name. > + */ > +int > +access_check_universe_object(user_access_t access, enum schema_object_type type, Nit: I'd s/type/object_type for consistency with object_name. > + const char *object_name); > + > static inline int > session_push(struct session *session, uint64_t sync, struct port *port) > {