[Asterisk-code-review] res pjsip exten state: Add config support for exten state pu... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: res_pjsip_exten_state: Add config support for exten state publishers.
......................................................................


Patch Set 1: Code-Review-1

(7 comments)

Minor findings.

https://gerrit.asterisk.org/#/c/2616/1/res/res_pjsip_exten_state.c
File res/res_pjsip_exten_state.c:

PS1, Line 90: 	/*! The body type to use for this publisher */
            : 	char *body_type;
Should comment that the string is stuffed after the member name[] string.


PS1, Line 94: 	/*! Whether context filtering is active */
            : 	unsigned context_filter;
unsigned int

You should reorder this struct to eliminate unnecessary padding between different types and their alignment.


PS1, Line 552: 	for (; (publisher = ao2_iterator_next(&publisher_iter)); ao2_ref(publisher, -1)) {
             : 		if ((publisher->context_filter && regexec(&publisher->context_regex, context, 0, NULL, 0)) ||
             : 		    (publisher->exten_filter && regexec(&publisher->exten_regex, exten, 0, NULL, 0))) {
             : 			continue;
             : 		}
             : 
             : 	}
I take it that after the if test, the loop body is a place holder for future code?  Maybe a comment that the loop is unfinished.


Line 681: 			ast_log(LOG_ERROR, "Could not filter context on publisher %s\n",
How about:
Could not build context filter '%s' on publisher %s\n
context, pub_id


Line 693: 			ast_log(LOG_ERROR, "Could not filter exten on publisher %s\n",
How about:
Could not build exten filter '%s' on publisher %s\n
exten, pub_id


PS1, Line 729: 	if (publishers) {
             : 		if (ao2_container_count(publishers)) {
             : 			ast_extension_state_del(0, exten_state_publisher_state_cb);
             : 		}
             : 
             : 		ao2_ref(publishers, -1);
             : 	}
This code can be:
ast_extension_state_del(0, exten_state_publisher_state_cb);
ao2_cleanup(publishers);
publishers = NULL;


Line 742: 	CHECK_PJSIP_MODULE_LOADED();
Existing bug that needs to be fixed in v13 and master:
Replace with CHECK_PJSIP_PUBSUB_MODULE_LOADED()

For this patch also need to add a check for:
if (!ast_module_check("res_pjsip_outbound_publish.so") {
  return AST_MODULE_LOAD_DECLINE;
}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7e630136dfc355073c1cadff8ad394a08523d78
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes