[Asterisk-code-review] chan sip: Support parsing of Q.850 reason header in SIP BYE ... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: chan_sip: Support parsing of Q.850 reason header in SIP BYE and CANCEL requests.
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

Review https://gerrit.asterisk.org/#/c/1620/ and https://gerrit.asterisk.org/#/c/1643/ are part of a patch series that need to be squashed into a single commit.

Please "git rebase -i" and squash the commits together as indicated in [1] after fixing the items found.  Then you should abandon the extra review that is no longer needed.

https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage#GerritUsage-UpdatingaReview

https://gerrit.asterisk.org/#/c/1643/1/channels/chan_sip.c
File channels/chan_sip.c:

Line 16132
I'm not sure removing this line is a good thing.

It is a change in behaviour for responses.  Responses now no longer have the hangup cause cleared by default in case there are no causes provided.

For BYE and CANCEL requests that do not provide a Reason header the cause is whatever is previously set.


Line 16141: 				ret=0;
Guidelines: Space around binary operators.


Line 16144: 					ast_verbose("Using Reason header for cause code: %d\n", ast_channel_hangupcause(pvt->owner));
Guidelines: Lines need to be wrapped at or before column 90.
Break the long line at ast_channel_hangupcause().


Line 24140: 		if (use_reason_header(p, req) != 0) {
There is no need to explicitly test against 0.  This is sufficient:
if (use_reason_header(...)) {
}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0389134dc53879387f05cb86a9340559bdb122db
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Christof Lauber <christof.lauber at annax.ch>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes