[Asterisk-code-review] loader: Convert reload classes to built-in modules. (asterisk[master])

Wed Mar 7 20:11:37 CST 2018

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/8390 )

Change subject: loader: Convert reload_classes to built-in modules.
......................................................................

Patch Set 1:

(10 comments)

I should be able to get an updated posted in the next couple days.

https://gerrit.asterisk.org/#/c/8390/1/include/asterisk/module.h
File include/asterisk/module.h:

https://gerrit.asterisk.org/#/c/8390/1/include/asterisk/module.h@295
PS1, Line 295: 	AST_MODPRI_CORE =               40,  /*!< A core module originally meant to start between preload and load. */
> Does this need it's own different priority value?
I don't think so, I'm not aware of any dependencies between timing modules and core modules.

If you'd like I can add 10 to AST_MODPRI_TIMING through AST_MODPRI_CDR_DRIVER so CORE modules will be loaded before timing and we'll still have 10 between each priority.

https://gerrit.asterisk.org/#/c/8390/1/main/Makefile
File main/Makefile:

https://gerrit.asterisk.org/#/c/8390/1/main/Makefile@21
PS1, Line 21: # Allow deletion of built-in modules without needing to modify this source.
> deletion?
As it is this supports deleting a couple core modules.  For example it's possible to delete cel.c or enum.c, the build will continue without error.  Deleting cel.c would mean that app_celgenuserevent and cel_* modules would be unable to load but Asterisk would still be able to load.  Same for enum.c, only func_enum would be effected by deleting that built-in module.

Maybe this could be modified to use $(shell $(GREP) -e AST_MODULE_INFO -l), but then GREP would be mandatory (probably not an issue).

https://gerrit.asterisk.org/#/c/8390/1/main/asterisk.c
File main/asterisk.c:

https://gerrit.asterisk.org/#/c/8390/1/main/asterisk.c@4548
PS1, Line 4548: 	check_init(ast_cc_init(), "Call Completion Supplementary Services");
> Call completion reads the ccss.conf file so it needs to be a built in modul
Ah not sure how I missed this.  Will fix in follow-up.

https://gerrit.asterisk.org/#/c/8390/1/main/asterisk.c@4550
PS1, Line 4550: 	/* We should avoid most config loads before this point as they can't use realtime. */
              : 	check_init(load_modules(1), "Module Preload");
              : 
              : 	/* Load remaining modules */
              : 	check_init(load_modules(0), "Module");
> Is there any need for two module loads now?  Or does this still need to be 
It's still possible to force modules to higher in the load order by using 'preload'.  I suspect we can eventually merge the two calls to load_modules so it builds a separate preload load priority list but that is not going to be part of this patch.

https://gerrit.asterisk.org/#/c/8390/1/main/cel.c
File main/cel.c:

https://gerrit.asterisk.org/#/c/8390/1/main/cel.c@1568
PS1, Line 1568: 		return AST_MODULE_LOAD_FAILURE;
> We should still be calling cel_engine_cleanup() i.e. unload_module() before
I think this is inconsistent.  We do not run unload_module on 'core stop now', which is basically what is happening when we exit with failure.  We definitely leak memory when we abort startup, if the next built-in module (after cel) fails to startup we never call the cel.c:unload_module, we just run things registered by ast_register_atexit and then terminate.  Ultimately having a bunch of calls to unload_module for every error path doesn't really accomplish anything useful.

https://gerrit.asterisk.org/#/c/8390/1/main/dsp.c
File main/dsp.c:

https://gerrit.asterisk.org/#/c/8390/1/main/dsp.c@2407
PS1, Line 2407: 	int res = _dsp_init(0);
> We should return early if res is nonzero rather than possibly registering t
That makes sense, will do.

https://gerrit.asterisk.org/#/c/8390/1/main/features.c
File main/features.c:

https://gerrit.asterisk.org/#/c/8390/1/main/features.c@1168
PS1, Line 1168: 	return res ? AST_MODULE_LOAD_FAILURE : AST_MODULE_LOAD_SUCCESS;
> We should be calling unload_module() if res is nonzero to cleanup on failur
See comments on cel.c.

https://gerrit.asterisk.org/#/c/8390/1/main/indications.c
File main/indications.c:

https://gerrit.asterisk.org/#/c/8390/1/main/indications.c@1141
PS1, Line 1141: 		return AST_MODULE_LOAD_FAILURE;
> We still should call indications_shutdown()/unload_module() here.
See cel.c.

https://gerrit.asterisk.org/#/c/8390/1/main/loader.c
File main/loader.c:

https://gerrit.asterisk.org/#/c/8390/1/main/loader.c@509
PS1, Line 509: 		mod = ast_calloc(1, sizeof(*mod) + strlen(info->name) + 1);
> This is going to get called before MALLOC_DEBUG/astmm.c has initialized for
Hmm I think I'll have to ast_std_calloc here, then when built-in modules are processed by modules_load I'll need to use ast_calloc to create a 'final' object and ast_std_free the one created here.

https://gerrit.asterisk.org/#/c/8390/1/res/res_config_sqlite.c
File res/res_config_sqlite.c:

https://gerrit.asterisk.org/#/c/8390/1/res/res_config_sqlite.c@1780
PS1, Line 1780: 	.requires = "cdr",
> Should it be extconfig?
This module is funny as it registers a realtime driver, loads config and registers a CDR backend.

Also looks like .support_level was missed, this module is deprecated.

-- 
To view, visit https://gerrit.asterisk.org/8390
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371a9a45064f20026c492623ea8062d02a1ab97f
Gerrit-Change-Number: 8390
Gerrit-PatchSet: 1
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 02:11:37 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180307/b2fcf840/attachment.html>