[Asterisk-code-review] loader: Add support for disabled modules. (asterisk[master])

[Richard Mudgett]

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

Change subject: loader: Add support for disabled modules.
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/10377/2/include/asterisk/module.h
File include/asterisk/module.h:

https://gerrit.asterisk.org/#/c/10377/2/include/asterisk/module.h@75
PS2, Line 75: 	 * system-wide stability.  Declined modules will prevent other any
s/other any/any other/


https://gerrit.asterisk.org/#/c/10377/2/include/asterisk/module.h@91
PS2, Line 91: 	/*!
            : 	 * \brief Module load is successful but disabled.
            : 	 *
            : 	 * Returning this value has the same effect as calling
            : 	 * ast_module_set_disabled(1) then returning AST_MODULE_LOAD_SUCCESS.
            : 	 * It is important that a module not unload if returning LOAD_DISABLED.
            : 	 * load_module will not be called again.  It is expected that a disabled
            : 	 * module can be enabled by modifying and reload the module's configuration.
            : 	 *
            : 	 * See \ref ast_module_set_disabled
            : 	 */
            : 	AST_MODULE_LOAD_DISABLED = 4,
What is the benefit of returning this at all?

Modules can (and already do) set themselves up in their own disabled state and return SUCCESS without this patch.

After further examination, this patch just does status reporting with things like the CLI "module show [like]" command.  However, for the status reporting to be accurate it would require changing all modules to return DISABLED (or call ast_module_set_disabled()) when the user has configured them to be disabled.  This is doable but will require diligence to prevent new modules from not reporting.

It would be more useful if the loader kept track of which dependencies were missing to report that in the CLI "module show [like]" command.

* Running - module is ready for use
* Declined - Module itself declined to load
* Missing dependencies - Module is missing dependencies or they are declined (may want to add which dependencies are preventing loading)

With the missing dependencies information we could make it so the dependent modules could attempt loading when the user fixes the dependent module's declined status.

res_smdi - declined
app_voicemail - missing dependency (res_smdi)

The user does a "module load res_smdi.so" after fixing the config error.
app_voicemail then automatically tries to load again if res_smdi's status becomes running.


https://gerrit.asterisk.org/#/c/10377/2/main/loader.c
File main/loader.c:

https://gerrit.asterisk.org/#/c/10377/2/main/loader.c@1427
PS2, Line 1427: 	int oldValue;
old_value
not camel case


https://gerrit.asterisk.org/#/c/10377/2/main/loader.c@2339
PS2, Line 2339: 		return "Run Declined";
Just "Declined"



-- 
To view, visit https://gerrit.asterisk.org/10377
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic44810ca6ce3deca30bceeb4eaa306d94f6b90e8
Gerrit-Change-Number: 10377
Gerrit-PatchSet: 2
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 04 Oct 2018 19:27:50 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181004/3e0c517e/attachment-0001.html>