[Asterisk-Dev] loader issues.

[Luigi Rizzo]

I think there are two issues in the current loading scheme.
First is a namespace pollution - by forcing each module 
have global symbols with 'common' names such as key, usecount,
reload, description we cannot easily run with, say, -Wshadow
or other stricter compiler checks.
Putting all callbacks in a single descriptor would ease life
to the compiler and the developer by reducing the namespace
pollution.

Secondly, in loader.c i notice the following comment:
        /* Resource modules are always loaded global and lazy */
                flags = (RTLD_GLOBAL | RTLD_LAZY);

I understand that in this way you can do dirty things such as
allowing module A to use module B's symbols and vice-versa,
but this makes really really easy to shoot yourself in the foot
at runtime.

In fact, both things seem to me dangerous for two reasons:
1) by using RTLD_LAZY, you defer symbol resolution at a later time,
   which is probably the opposite of what you want -- surely
   you want to know of unresolved symbols at the time you load a
   module and not much later perhaps in the middle of some unrelated
   action;

2) RTLD_GLOBAL opens the door to unwanted bugs, e.g. a module defining
   global symbols by mistake, and making them available to others in
   unwanted ways.

How about providing a controlled mechanism through which a module
can export symbols in a controlled way, so that other modules that
need them also have to signal its use through a usecount call.

E.g. something like

	struc exported_symbols {
		const char *name;
		void *value;
	}

	struct module_desc {
		int (*load_module)(void);
		int (*unload_module)(void);
		int (*usecount)(void);
		const char *description;
		const char *key;
		struc exported_symbols *s;
	}

and in a module, say res/res_monitor.c, which exports ast_monitor_start

	static struct exported_symbols ee[] = {
		{ "ast_monitor_start", ast_monitor_start},
		{ NULL, NULL }
	}
	struct module_desc module_desc = {
		.reload = NULL, /* XXX missing ? */
		.load_module = monitor_load_module,
		.unload_module = monitor_unload_module,
		.description = "Call Monitoring Resource",
		.key = ASTERISK_GPL_KEY,
		.usecount = my_usecount,
		.s = ee
	}

now a trivial loader function could be used to do a lookup of
the desired symbol by name, and increment the usecount of the
corresponding module. Way way safer than what we have now.

void *sym_lookup(const char *sym)
{
        struct module *m;
        struct exported_symbols *e;

        /* lock modlock */
        for (m = module_list; m ; m = m->next)
                for (e = m->s; e && e->name; e++)
                        if (!strcmp(e->name, sym) {
                                /* update m->usecount */
                                /* unlock modlock */
                                return e->value;
                        }
        /* not found */
        /* unlock modlock */
        return NULL;
}

Users then would do

	if (ast_monitor_start == NULL)
		ast_monitor_start = sym_lookup("ast_monitor_start")
	if (ast_monitor_start == NULL) {
		ast_log(" ... sorry... would you like to panic instead ?");
	else
		ast_monitor_start( ... );

I understand that dereferencing the pointer the first time
has some overhead (you need to do a lookup), but in the
current state of affairs you risk a panic, which is not nice either.

	cheers
	luigi