[Asterisk-code-review] res_pjsip_rfc3326: Q.850 reason code for CANCEL is set to AST_CAUSE_N... (asterisk[18])

[George Joseph]

Attention is currently required from: Joshua Colp, Mark Petersen.
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16344 )

Change subject: res_pjsip_rfc3326: Q.850 reason code for CANCEL is set to AST_CAUSE_NORMAL_CLEARING
......................................................................


Patch Set 5: Code-Review-1

(4 comments)

Patchset:

PS5: 
Just thought of something else...  What happens if you're using ARI to make the call?  That doesn't go through app_dial.


File apps/app_dial.c:

https://gerrit.asterisk.org/c/asterisk/+/16344/comment/6e49c13e_7dcb9d6a 
PS5, Line 2873: 			cancelcause = ast_channel_hangupcause(chan);
wait_for_answer is pretty complicated.  Are you sure just using peer and to is enough to positively indicate that the caller hung up?  You may want to do the determination in wait_for_answer and add a field to "pa" with the hangup cause.  There's already code in there that deals with the caller hanging up.


https://gerrit.asterisk.org/c/asterisk/+/16344/comment/228c33c3_87a7cab0 
PS5, Line 3360: ast_channel_hangupcause(chan) == AST_CAUSE_ANSWERED_ELSEWHERE
              : 		|| ast_test_flag64(&opts, OPT_CANCEL_ELSEWHERE)
              : 		? AST_CAUSE_ANSWERED_ELSEWHERE : cancelcause
I know it was like this before but i had a hard time deciphering what this was doing.  Think you could move this before the hanguptree call and make it a bit clearer?


File res/res_pjsip_rfc3326.c:

https://gerrit.asterisk.org/c/asterisk/+/16344/comment/858982fc_6d8b932d 
PS5, Line 80: 	if (ast_channel_hangupcause(session->channel) == AST_CAUSE_NOTDEFINED &&
            : 		!pjsip_method_cmp(&rdata->msg_info.msg->line.req.method, &pjsip_cancel_method)) {
            : 			ast_debug(1, "Channel '%s', setting hangup cause: %d\n", ast_channel_name(session->channel), AST_CAUSE_NORMAL_CLEARING);
            : 			ast_channel_hangupcause_set(session->channel, AST_CAUSE_NORMAL_CLEARING);
            : 	}
            : 
Is this even still needed with the logic in Dial?



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

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: Ib653aec2282f55b59d87484391cc07c8e6612b89
Gerrit-Change-Number: 16344
Gerrit-PatchSet: 5
Gerrit-Owner: Mark Petersen <bugs.digium.com at zombie.dk>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Mark Petersen <bugs.digium.com at zombie.dk>
Gerrit-Comment-Date: Tue, 05 Oct 2021 13:54:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211005/733ce4d6/attachment-0001.html>