Re: [tarantool-patches] Re: [PATCH v1 1/1] box: support reload whole module

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

On Wed, Jul 11, 2018 at 03:46:56PM +0300, Vladimir Davydov wrote:
> On Tue, Jul 10, 2018 at 09:49:14AM +0300, Kirill Shcherbatov wrote:
> > @@ -318,7 +320,19 @@ module_reload(const char *package, const char *package_end, struct module **modu
> >  	struct func *func, *tmp_func;
> >  	rlist_foreach_entry_safe(func, &old_module->funcs, item, tmp_func) {
> >  		struct func_name name;
> > -		func_split_name(func->def->name, &name);
> > +		const char *func_name = func->def->name;
> > +		func_split_name(func_name, &name);
> > +
> > +		/*
> > +		 * Allow to reload only functions that belongs to
> > +		 * current user. Skip other.
> > +		 */
> > +		struct func *dummy;
> > +		if (check_access &&
> > +		    box_func_check_access(func_name, strlen(func_name),
> > +					  &dummy) != 0)
> > +			continue;
> > +
> 
> AFAIU a user may reload a whole module only if he has the global EXECUTE
> privilege (because we don't have such an entity as module in our data
> dictionary to grant access rights for). access_check_func(), which is
> called by func_reload(), already checks the global EXECUTE privilege and
> returns 0 (success) if it is set, no matter if the function was found or
> not. So all you have to do is call module_reload() from func_reload() if
> access_check_func() returned func = NULL, no?

During our verbal discussion with Kirill, he noticed that the fact that
box.schema.func.reload() reloads the whole module when it is passed a
function name is rather confusing: the user may not know that and call
box.schema.func.reload() once per each used function, in which case he
will effectively reload the whole module that contains those functions
multiple times, which is pointless. Turns out Kirill isn't the only one
who finds such an API weird, see

  https://github.com/tarantool/tarantool/issues/910#issuecomment-331435227

I talked to Kostja and he doesn't mind banning this API, i.e. making
box.schema.func.reload() interpret its argument only as a module name.

So let's rework this patch so that box_func_reload() directly calls
module_reload(), and remove func_reload() altogether. As for the access
checks, box_func_reload() should work only if the caller has the global
execute privilege.