Re: [Tarantool-patches] [PATCH v20 4/7] box/module_cache: introduce modules subsystem

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Mon, Apr 05, 2021 at 05:52:47PM +0200, Vladislav Shpilevoy wrote:
> > +/**
> > + * Helpers for cache manipulations.
> > + */
> > +static void *
> 
> 1. It returns struct module, therefore the return type must be
> 'struct module *', not 'void *'. The same for cache_find() in box.lib
> implementation.

OK

> > +static int
> > +cache_put(struct module *m)
> > +{
> > +	const struct mh_strnptr_node_t nd = {
> > +		.str	= m->package,
> > +		.len	= m->package_len,
> > +		.hash	= mh_strn_hash(m->package, m->package_len),
> > +		.val	= m,
> > +	};
> > +
> > +	mh_int_t e = mh_strnptr_put(module_cache, &nd, NULL, NULL);
> 
> 2. Put() silently replaces the old value if it is present. I would
> recommend to use the next to the last argument to get the old value
> and ensure it is mh_end() via an assertion/panic. The same for the other
> new put() functions in the other commits.

Sure

> 
> > +	if (e == mh_end(module_cache)) {
> > +		diag_set(OutOfMemory, sizeof(nd), "malloc",
> > +			 "module_cache node");
> > +		return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void
> > +cache_del(struct module *m)
> > +{
> > +	const char *str = m->package;
> > +	size_t len = m->package_len;
> > +
> > +	mh_int_t e = mh_strnptr_find_inp(module_cache, str, len);
> > +	if (e != mh_end(module_cache)) {
> 
> 3. Maybe this must be an assertion/panic. I don't see a valid case when
> del() is called on an already deleted module. The same for the other
> new del() functions in the other commits.

When we put the module in the cache and something is failed we call
generic module_unload which in turn calls cache_del

module_load
  ...
  m = module_new(package, package_len, path);
  if (m != NULL && cache_put(m) != 0) {
    module_unload(m);
      --> module_unref
            if (--m->refs == 0) {
              cache_del(m);

this is done for simplicity. So calling cache_del with
module which is not in cache is fine.

> > +
> > +		/*
> > +		 * In case of cache hit we may reuse existing
> > +		 * module which speedup load procedure.
> > +		 */
> > +		module_attr_fill(&attr, &st);
> > +		if (memcmp(&attr, &m->attr, sizeof(attr)) == 0) {
> 
> 4. Please, add a static assertion, that sizeof(module_attr) == 40.
> Otherwise somebody might add a new attribute, which won't be uint64_t,
> and would break the comparison without noticing. Also you can make the
> attributes be stored as a byte array char[40] to make it impossible to
> add any padding into it. Also you can compare the attributes one by
> one.

Not needed anymore.

static void
module_attr_fill(struct module_attr *attr, struct stat *st)
{
-->	memset(attr, 0, sizeof(*attr));

any possible padding is explicitly cleared. Initially I though
of using __packed attribue or something but at the end realised
that using explicit cleanup is a way more robust.

> > +void
> > +module_free(void)
> > +{
> > +	mh_int_t e;
> > +
> > +	mh_foreach(module_cache, e) {
> > +		struct module *m = mh_strnptr_node(module_cache, e)->val;
> > +		module_unload(m);
> 
> 5. As I said in the previous review, it does not make much sense.
> If there are any not unloaded modules, and they try to unload later,
> they will see module_cache == NULL and will crash.
> 
> Also you can't do unload here, because the module_cache itself does
> not keep any references. All the unloads must be done by the module
> objects owners. Not by module_cache on its own. For example, if there
> is a module having a single reference and used in some other subsystem,
> your unload will free it and make it memory invalid. That will crash
> in case the module owner will try to access it again.
> 
> There should be a panic-check that the module cache is empty already.

Not at all. You can exit tarantool via Ctrl+D inside console and
modules won't be empty and we should clean them up. So I can and
I should unload modules here. Vlad, this is _exit_ path called when
we're exiting tarantool. What I'm missing?

> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> 
> 6. You don't need these headers in module_cache.h. They are
> needed only in the .c file.

Yes, thanks for pointing.