[Asterisk-code-review] dialplan functions: wrong srtp useage report of a dialplan f... (asterisk[13])

[Richard Mudgett]

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

Change subject: dialplan_functions: wrong srtp useage report of a dialplan function
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

Good first attempt.  The code appears to work as is but can be better expressed and needs to follow the coding guidelines.

https://gerrit.asterisk.org/#/c/9807/1//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/9807/1//COMMIT_MSG@7
PS1, Line 7: dialplan_functions: wrong srtp useage report of a dialplan function
s/useage/usage/


https://gerrit.asterisk.org/#/c/9807/1/channels/pjsip/dialplan_functions.c
File channels/pjsip/dialplan_functions.c:

https://gerrit.asterisk.org/#/c/9807/1/channels/pjsip/dialplan_functions.c@533
PS1, Line 533: 	  if(media->srtp) {
             : 	    struct ast_sdp_srtp *srtp = media->srtp;
             : 	    int flag = ast_test_flag( srtp, AST_SRTP_CRYPTO_OFFER_OK);
             : 	    snprintf(buf, buflen, "%d", flag ? 1 : 0);
             : 	  } else {
             : 	    snprintf(buf, buflen, "%d", 0);
             : 	  }		
This code is better expressed:

snprintf(buf, buflen, "%d",
    (media->srtp && ast_test_flag(media->srtp, AST_SRTP_CRYPTO_OFFER_OK)) ? 1 : 0);


Otherwise you have several coding guideline problems.
* Use tabs not spaces to indent.
* Space after if: if ()
* No space after open paren: ast_test_flag(srtp,...
* Trailing whitespace indicated by a red blob if the code isn't highlighted by a comment as happened here.

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


https://gerrit.asterisk.org/#/c/9807/1/main/sdp_srtp.c
File main/sdp_srtp.c:

https://gerrit.asterisk.org/#/c/9807/1/main/sdp_srtp.c@a353
PS1, Line 353: 
Ok.  I understand why you removed this.  Setting this flag is actually redundant.  It was set by the earlier SRTP negotiation that wasn't changed during the current renegotiation.

Removing this deserves a mention in the commit message.  Otherwise, someone could be puzzled why it was removed at a later time.



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I29dc2843cf4e5ae2604301cb4ff258f1822dc2d7
Gerrit-Change-Number: 9807
Gerrit-PatchSet: 1
Gerrit-Owner: Salah Ahmed <txrubel at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 02 Aug 2018 20:54:57 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180802/c99cf90b/attachment.html>