[Asterisk-code-review] res_pjsip_aoc: New module for sending advice-of-charge with chan_pjsip (asterisk[master])

[Joshua Colp]

Attention is currently required from: Michael Kuron.

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/19460 )

Change subject: res_pjsip_aoc: New module for sending advice-of-charge with chan_pjsip
......................................................................


Patch Set 2: Code-Review-1

(9 comments)

Patchset:

PS2: 
This needs to have a CHANGES entry[1] added. It also needs to be configurable on the PJSIP endpoint, and default to disabled. Test coverage should also be added for it.

[1] https://wiki.asterisk.org/wiki/display/AST/CHANGES+and+UPGRADE.txt


File channels/chan_pjsip.c:

https://gerrit.asterisk.org/c/asterisk/+/19460/comment/59f33659_4ae5566f 
PS2, Line 1834: 	case AST_CONTROL_AOC:
Instead of modifying chan_pjsip and adding this callback mechanism, can a framehook be used instead in the implementation module?


File res/res_pjsip_aoc.c:

https://gerrit.asterisk.org/c/asterisk/+/19460/comment/c7f97780_2d0e267e 
PS2, Line 22: 	<support_level>core</support_level>
extended


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/8e2cd01e_31eb3e0d 
PS2, Line 62: 	node->content.ptr = NULL;
If you use PJ_POOL_ZALLOC_T then it will be zeroed out already.


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/03bb2f86_a6f92520 
PS2, Line 158: 			1024, 1024);
Where did the 1024 come from?


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/be27e3e9_459d5bd6 
PS2, Line 390: 			pbx_builtin_setvar_helper(ast, "AOCS", xml);
Using channel variables for this purpose is generally frowned upon. A datastore should exist instead to store persistent information on the channel.


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/cc9adf29_4ed1c7cd 
PS2, Line 467: 	if (tdata_sdp_info->sdp) {
What happens if this is already multipart?


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/362c92d7_e2aeaddf 
PS2, Line 491: static pj_status_t aoc_outgoing_response(pjsip_tx_data *tdata)
Why does this need to be done instead of using a session supplement?


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/9598cfbc_23a0bb2f 
PS2, Line 569: 	.support_level = AST_MODULE_SUPPORT_CORE,
extended



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iebb7ad0d5f88526bc6629d3a1f9f11665434d333
Gerrit-Change-Number: 19460
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Kuron <m.kuron at gmx.de>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Michael Kuron <m.kuron at gmx.de>
Gerrit-Comment-Date: Fri, 28 Oct 2022 12:51:54 +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/20221028/4e13066a/attachment.html>