[Asterisk-code-review] Remove redundant module checks and references. (asterisk[master])

[Corey Farrell]

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

Change subject: Remove redundant module checks and references.
......................................................................


Patch Set 4:

(3 comments)

I'll address the res_pjsip_session requirements soon (or maybe in a separate commit).  For now I don't really want to rebase everything else.  This review only depends on the first review (7873) so once that is merged this can be rebased onto master instead of being in the series.

https://gerrit.asterisk.org/#/c/7989/4/apps/app_confbridge.c
File apps/app_confbridge.c:

https://gerrit.asterisk.org/#/c/7989/4/apps/app_confbridge.c@2389
PS4, Line 2389: 	if (ast_test_flag(&user.u_profile, USER_OPT_JITTERBUFFER)) {
              : 		ast_func_write(chan, "JITTERBUFFER(adaptive)", "default");
              : 	}
              : 
              : 	if (ast_test_flag(&user.u_profile, USER_OPT_DENOISE)) {
> These checks are still needed.  They are not to prevent loading because a d
This change was intentional, if the admin enables jitterbuffer or denoise and does not load the required modules we should print a warning.

I see this situation no differently than putting 'same => n,Set(JITTERBUFFER(adaptive)=default)' into dialplan.  That doesn't cause a warning at pbx_config load, it produces a warning each time it is run until the admin loads func_jitterbuffer or changes the dialplan.


https://gerrit.asterisk.org/#/c/7989/4/res/res_hep_pjsip.c
File res/res_hep_pjsip.c:

https://gerrit.asterisk.org/#/c/7989/4/res/res_hep_pjsip.c@235
PS4, Line 235: 	if (!hepv3_is_loaded()) {
             : 		ast_log(AST_LOG_WARNING, "res_hep is disabled; declining module load\n");
             : 		return AST_MODULE_LOAD_DECLINE;
             : 	}
> This shouldn't be needed if res_hep declines to load when it is disabled.
Personally I don't like the current way this is done but I made these minimum changes (removing the ast_module_check's).

I think we should stop declining load and instead conditionally register/unregister logging_module.  Also somehow monitor the enabled/disabled status of res_hep and manage logging_module based on that.  I'm not sure if that's something that should be master only or all branches, but it seems like something that should be in a separate commit.


https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_diversion.c
File res/res_pjsip_diversion.c:

https://gerrit.asterisk.org/#/c/7989/4/res/res_pjsip_diversion.c@429
PS4, Line 429: 	.requires = "res_pjsip",
> Include res_pjsip_session too since this module used CHECK_PJSIP_SESSION_MO
ast_sip_session_register_supplement and ast_sip_session_unregister_supplement are both in res_pjsip.so so my symbol usage scan did not find these dependencies (script attached to ASTERISK-26245).

This is one of the few exceptions where a module with global symbols is required even though we do not link to it.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I332a6e8383d4c72c8e89d988a184ab8320c4872e
Gerrit-Change-Number: 7989
Gerrit-PatchSet: 4
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: Sun, 21 Jan 2018 18:34:41 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180121/d2057b89/attachment.html>