Re: [Tarantool-patches] [PATCH v10 2/4] module_cache: move module handling into own subsystem

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Thu, Nov 12, 2020 at 11:54:01PM +0100, Vladislav Shpilevoy wrote:
> 
> 1. I see it is not just moved. I by luck noticed, that the code
> looks suspiciously different. And after a closed look I
> found that you did some renames, and functional changes. As I
> said in the other email - please, don't do that. It is extremely
> hard to spot what you changed or renamed in this huge diff with
> lots of moves. I will commment below some changes that I noticed.
> 
> But I ask you to split the renames and functional changes into
> separate commits, so as they could be reviewer properly.

Sure.

> > +
> > +/**
> > + * Find a path to a module using Lua's package.cpath.
> > + */
> > +static int
> > +module_find(struct module_find_ctx *ctx)
> > +{
> 
> 2. In the original code the function took package name and
> path, which was way more clear than a ctx. Which in the
> old code was constructed inside of this function.
> 
> But you somewhy moved it to the caller code, to module_load(),
> which creates the ctx just to call module_find(), and never
> uses it again. So what was the point? I honestly do not
> understand.

I thought it gonna be a bit more readable. If you prefer old style
then sure I'll keep it.

> > +	mod_sym->addr = module_sym(module, d.sym);
> > +	if (mod_sym->addr == NULL)
> > +		return -1;
> 
> 3. Is it correct, that you just removed the bugfix you did
> in the previous commit? Here, if the module was loaded first
> time, you again do not unload it.

Sigh... you're right, the chunk is sneaked in. Thanks for spotting!

> > +
> > +	mod_sym->module = module;
> > +	rlist_add(&module->mod_sym_head, &mod_sym->list);
> > +	return 0;
> > +}
> > diff --git a/src/box/module_cache.h b/src/box/module_cache.h
> > new file mode 100644
> > index 000000000..8b2510ec9
> > --- /dev/null
> > +++ b/src/box/module_cache.h
> > @@ -0,0 +1,158 @@
> > +/*
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> > + */
> > +
> > +#pragma once
> > +
> > +#include <small/rlist.h>
> > +
> > +#if defined(__cplusplus)
> > +extern "C" {
> > +#endif /* defined(__cplusplus) */
> > +
> > +
> > +/**
> > + * API of C stored function.
> > + */
> > +
> > +struct port;
> > +
> > +struct box_function_ctx {
> > +	struct port *port;
> > +};
> > +
> > +typedef struct box_function_ctx box_function_ctx_t;
> > +typedef int (*box_function_f)(box_function_ctx_t *ctx,
> > +			      const char *args,
> > +			      const char *args_end);
> > +
> > +/**
> > + * Function name descriptor: a symbol and a package.
> > + */
> > +struct func_name_desc {
> 
> 4. It is never used out of module_cache.c. Why do you need it
> in the header?

I think we gonna rework modules engine more a bit later, in particular
we will need to test symbols early on function definition and I've
a plan to reuse this structure. But for this patchset it looks out
of context, so I'll move it back to source file.

> > +
> > +/**
> > + * Dynamic shared module.
> > + */
> > +struct module {
> > +	/**
> > +	 * Module dlhandle.
> > +	 */
> > +	void *handle;
> > +	/**
> > +	 * List of associated symbols (functions).
> > +	 */
> > +	struct rlist mod_sym_head;
> 
> 5. I see you renamed it from 'funcs'. It was much better, tbh.
> I ask you to keep the old name. Or rename it to 'syms', if you like
> the 'sym' name so much. But when you named it 'head', you stated
> that this is a head of the list, its first element. However it is
> not. This rlist is the list itself. If you will call rlist_entry()
> on this member, you won't get a valid module_sym object.
> 
> Also since you decided to change the comment, it would be better
> to say what objects are stored here. Their type.

We already have "funcs".

src/box/schema.cc:static struct mh_i32ptr_t *funcs;
src/box/schema.cc:static struct mh_strnptr_t *funcs_by_name;

Having it here simply confusing. We should be able to greap easily
the context of variables, moreover the engine operates with addresses
of symbols not functions. The name exposes the meaning.

To the "head" name -- this *is* the head of the list. The entries
are linked into this list. And a head of a list is *not* its first
element, I'm not sure where you get this assumption from. And it
never been. The first element is

inline static struct rlist *
rlist_first(struct rlist *head)
{
	return head->next;
}

I could rename it to syms_list if you prefer. But please no "funcs".
This really makes grepping a way more worse :(

> > +/**
> > + * Callable symbol bound to a module.
> > + */
> > +struct module_sym {
> > +	/**
> > +	 * Anchor for module membership.
> > +	 */
> > +	struct rlist list;
> 
> 6. But it is not a list. It is just a link of the intrusive list.
> It is not a self-sufficient list. Also since you changed the
> comment anyway, I ask you to say what is the list this
> anchor is stored in (module.funcs).

I'll rename it to "item". Sounds ok?

You know I would prefer to not point in comment where exactly this
entries are linked into: the name of keeping structure may change,
moreover we might introduce some temporary storage later or something
else and such comment become misleading :( But since you insist I'll
add.

> > +/**
> > + * Reload a module and all associated symbols.
> > + *
> > + * @param str function name, e.g. "module.submodule.function".
> > + * @param[out] d parsed symbol and a package name.
> 
> 7. There are no such parameters as 'str' and 'd'.

Ouch, sneaked from merging :( Thanks!