[PATCH v3 3/6] box: rework func object as a function frontend
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jun 18 16:23:20 MSK 2019
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)
> {
More information about the Tarantool-patches
mailing list