[Asterisk-code-review] pjsip: Add option global/regcontext (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: pjsip:  Add option global/regcontext
......................................................................


Patch Set 2: Code-Review-1

(10 comments)

Mostly formatting issues.  Dan you should take a look at the Coding Guidelines on the wiki [1].  Many of the issues I've flagged are listed on that page, most are pretty simple.

[1] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines

https://gerrit.asterisk.org/#/c/1984/2//COMMIT_MSG
Commit Message:

Line 13: ASTERISK-25670
Please make this line 'ASTERISK-25670 #close' - assuming this commit will resolve that JIRA issue.


https://gerrit.asterisk.org/#/c/1984/2/CHANGES
File CHANGES:

Line 18:  * Added new global option (regcontext) to pjsip. When set, Asterisk will 
Extra space at the end of this and the next line.


https://gerrit.asterisk.org/#/c/1984/2/configs/samples/pjsip.conf.sample
File configs/samples/pjsip.conf.sample:

Line 889: ; NoOp priority 1 extension for a given endpoint who registers or unregisters 
Space at the end.


https://gerrit.asterisk.org/#/c/1984/2/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

Line 2048:  */
Please add to this comment on a new line \since 13.8.0.  This helps any module developer know that this function is incompatible with 13.0.0 - 13.7.x.

This needs a note about free'ing the return - you can copy this from the ast_sip_get_debug comment.


https://gerrit.asterisk.org/#/c/1984/2/res/res_pjsip.c
File res/res_pjsip.c:

Line 1275:                                         <synopsis>When set, Asterisk will dynamically create and destroy a NoOp priority 1 extension for a given 
Space at the end.


https://gerrit.asterisk.org/#/c/1984/2/res/res_pjsip/config_global.c
File res/res_pjsip/config_global.c:

Line 154:         return res;
Nit-pick: a blank line before this return would be nice.


https://gerrit.asterisk.org/#/c/1984/2/res/res_pjsip/pjsip_configuration.c
File res/res_pjsip/pjsip_configuration.c:

Line 125: 		if ( strcmp( regcontext, "") != 0 ) {
Please use: if (!ast_strlen_zero(regcontext)) {

That function is more efficient for checking blankness of a string.  Also coding guidelines do not allow any space after '(' or before ')'.


Line 126:                         if (!ast_exists_extension(NULL, regcontext, ast_endpoint_get_resource(endpoint), 1, NULL)) {
All indentation needs to be done with tabs, not spaces.


Line 127:                                 ast_add_extension(regcontext, 1, ast_endpoint_get_resource(endpoint), 1, NULL, NULL, "Noop", ast_strdup(ast_endpoint_get_resource(endpoint)), ast_free_ptr, "SIP");
This line is far too long, needs to be broken up.


Line 136: 		if ( strcmp( regcontext, "") != 0 ) {
!ast_strlen_zero here too.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib1530c5b45340625805c057f8ff1fb240a43ea62
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Daniel Journo <dan at keshercommunications.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-HasComments: Yes