Re: [PATCH v3 3/6] box: rework func object as a function frontend

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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)
>  {