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

[Richard Mudgett]

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

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


Patch Set 1: Code-Review-1

(11 comments)

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?


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?
Should that be addition?


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 module too.

ast_cc_init() calls initialize_cc_devstate_map() which reads ccss.conf.


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 done to sort of force other modules to load/start earlier?


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 exiting with failure.


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 the test commands.  Or we could simply eliminate res:

if (_dsp_init(0) {
   return AST_MODULE_LOAD_FAILURE;
}

AST_TEST_REGISTER...

return AST_MODULE_LOAD_SUCCESS;


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 failure.

if (res) {
   unload_module();
   return AST_MODULE_LOAD_FAILURE;
}
return AST_MODULE_LOAD_SUCCESS;


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.


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 the built-in modules.

We will have to use ast_std_calloc()/ast_std_free() for these specific allocations.  My always use MALLOC_DEBUG patch makes the ast_calloc() function check and ast_log() allocation failures which uses the logger system that hasn't been initialized yet either.


https://gerrit.asterisk.org/#/c/8390/1/main/loader.c@1459
PS1, Line 1459: 			/* Built-in modules cannot be unloaded. */
s/be unloaded/be manually unloaded/


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?



-- 
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: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 08 Mar 2018 00:14:09 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180307/fb85087d/attachment-0001.html>