[Asterisk-code-review] res pjsip mwi: fix unsolicited mwi blocks PJSIP stack (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: res_pjsip_mwi: fix unsolicited mwi blocks PJSIP stack
......................................................................


Patch Set 4: Code-Review-1

(15 comments)

https://gerrit.asterisk.org/#/c/3320/4/res/res_pjsip/config_global.c
File res/res_pjsip/config_global.c:

Line 48: #define DEFAULT_MWI_TPS_QUEUE_LOW (AST_TASKPROCESSOR_HIGH_WATER_LEVEL * 9)/10
Make the default -1 to indicate 90% of high water level.  This way you don't also have to set the low water level if you change the high water level.


Line 91: 		unsigned int tps_queue_low;
Make int so you can use -1 for 90% of high water level.


Line 94: 	} mwi;
The new options need to be documented in res_pjsip.c XML global section to pass tests.

Also an alembic script is needed for these new parameters.


Line 346: unsigned int ast_sip_get_mwi_tps_queue_low(void)
Make return int to use -1 as 90%.


PS4, Line 518: 	ast_sorcery_object_field_register(sorcery, "global", "mwi_tps_queue_low",
             : 		default_mwi_tps_queue_low,
             : 		OPT_UINT_T, 0, FLDSET(struct global_config, mwi.tps_queue_low));
Change to an OPT_INT_T to allow entering -1 as 90% of high water level.  There should be an example of the OPT_INT_T format registration somewhere.


https://gerrit.asterisk.org/#/c/3320/4/res/res_pjsip_mwi.c
File res/res_pjsip_mwi.c:

Line 211: 		ast_log(AST_LOG_WARNING, "The MWI serializer pool is not inizialazed. Can not set alert levels\n");
This is going to redundantly warn every time the global section type is reloaded if the pool fails to get initialized.  You have already complained if the pool failed to get setup.


Line 215: 	tps_queue_high = (long) ast_sip_get_mwi_tps_queue_high();
The cast shouldn't be necessary unless the compiler is set to really annoying.


Line 217: 		ast_log(AST_LOG_WARNING, "Invalid the taskprocessor high water alert trigger level '%ld'\n", tps_queue_high);
Break long line at the tps_queue_high parameter.


Line 221: 	tps_queue_low = (long) ast_sip_get_mwi_tps_queue_low();
Cast not necessary when it is changed to int to allow for -1 setting.


Line 222: 	if (tps_queue_low <= 0 || tps_queue_low >= tps_queue_high) {
Allow -1 to be configured for 90% of high.

if (tps_queue_high < tps_queue_low) then error and set to -1 default.

Zero is allowable for the low water mark to clear when the queue is emptied.  low==high is allowable because the alert is triggered when the queue reaches high and cleared when it comes back down to low.


Line 223: 		ast_log(AST_LOG_WARNING, "Invalid the taskprocessor low water clear alert level '%ld'\n", tps_queue_low);
Break long line at the tps_queue_low parameter.


Line 228: 		if (mwi_serializer_pool[idx]) {
This check isn't necessary.  The pool is either setup or it isn't.  If it isn't then the check at the start of this function is sufficient.


Line 230: 				ast_log(AST_LOG_WARNING, "failed to set alert levels for mwi serializer pool #%d.\n", idx);
s/failed/Failed/
s/mwi/MWI/

Break long line at the idx parameter.


Line 1298: 		mwi_serializer_pool_shutdown();
The shutdown is redundant.  mwi_serializer_pool_setup() has already done this if it fails.


PS4, Line 1299: 	} else {
              : 		mwi_serializer_set_alert_levels();
This is not needed as the global_observer will do this when the global section type is loaded.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c8ecb82c249eb887930980a800c9f87f28f861a
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes