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

[Matt Jordan]

Matt Jordan 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:

(10 comments)

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

Line 16124: static int parse_reason_header(struct sip_pvt *pvt, struct sip_request *req)
I'd rename this to "use_reason_header" or something similar, as we aren't just parsing the reason header - we're using it to set the hangup cause code on the channel.


Line 16128: 	int ret=FALSE;
Generally, while chan_sip does use it in certain places, I'd stay away from using the FALSE/TRUE.

Returning 0 on success, -1 on failure is a standard idiom throughout Asterisk.


Line 16129: 	if (owner) {
It's always a good idea to reduce indentation wherever possible. In this case, you can bail out of this entire function by simply doing:

if (!owner) {
        return -1;
}


Line 16127: 	owner = pvt->owner;
          : 	int ret=FALSE;
          : 	if (owner) {
There's a number of coding violations in this patch set. Please do review the coding guidelines on the wiki:

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


Line 16131: 		ast_channel_hangupcause_set(owner, 0);
This clears the hangupcause unilaterally, which is not correct based on the purpose of this function. In fact, I don't think we should manipulate the hangupcause at all on the channel unless we are certain the Q850 reasons should be used.

The code this is copied from resets the hangupcause code based on the SIP response:


		if (!ast_channel_hangupcause(owner))
			ast_channel_hangupcause_set(owner, hangup_sip2cause(resp));

Which you clearly don't need to do here, since you are doing this on requests, not responses. As well, this was done originally to tell if the hangup cause code was set or not based on the reason header; that no longer needs to be done at all since your function tells the calling code whether or not it has set the hangup cause code on the channel.


Line 24138: 		if (!parse_reason_header(p, req))
Instead of returning FALSE/TRUE, use the standard C idiom of 0 implies success, -1 or 1 implies failure. That would change this to:

if (parse_reason_header(p, req)) {
    /* Use the SIP cause */
    ast_channel_hangupcause_set(owner, hangup_sip2cause(resp));
}


Line 24138: 		if (!parse_reason_header(p, req))
          : 			ast_channel_hangupcause_set(owner, hangup_sip2cause(resp));
Even single line statements should have curly braces. Extremely old code in Asterisk does not always have this; all newer code should.


Line 26432: 	
Remove extraneous white space.


Line 26449: 		int cause = ast_channel_hangupcause(p->owner);
          : 		sip_queue_hangup_cause(p, cause);
There's no reason to store this in a local variable:

sip_queue_hangup_cause(p, ast_channel_hangupcause(p->owner));


Line 26670: 		int cause = ast_channel_hangupcause(p->owner);
          : 		sip_queue_hangup_cause(p, cause);
And same finding here about not needing to store the cause in a local variable.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifeed95a9c7c7fa86a67d0c512f3e75f28f86e66c
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: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes