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

[Michael Kuron]

Attention is currently required from: Joshua Colp.

Michael Kuron 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 3:

(9 comments)

Patchset:

PS2: 
> This needs to have a CHANGES entry[1] added. […]
Is there a way to register new endpoint settings without having to modify chan_pjsip?

Also, I may need some help with respect to adding test coverage.


File channels/chan_pjsip.c:

https://gerrit.asterisk.org/c/asterisk/+/19460/comment/38145190_6bd26746 
PS2, Line 1834: 	case AST_CONTROL_AOC:
> Instead of modifying chan_pjsip and adding this callback mechanism, can a framehook be used instead  […]
Done


File res/res_pjsip_aoc.c:

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


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


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/e15d1566_4e12ca16 
PS2, Line 158: 			1024, 1024);
> Where did the 1024 come from?
Latest commit changes it to 2048. I observed that about 1800 bytes from the pool are used, so rounding that up to 2048 seems like a good estimate to ensure that both no resizing is needed and not much memory is wasted.


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/13c4366a_b0270745 
PS2, Line 390: 			pbx_builtin_setvar_helper(ast, "AOCS", xml);
> Using channel variables for this purpose is generally frowned upon. […]
Done


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/8ad44475_eafc39ed 
PS2, Line 467: 	if (tdata_sdp_info->sdp) {
> What happens if this is already multipart?
Latest commit changes it to reuse an existing multipart body


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/37ff5155_beb6a113 
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?
In my testing, aoc_bye_supplement.outgoing_response never got triggered. So I had to use a pjsip_module instead of an ast_sip_session_supplement to make sure that aoc_bye_outgoing_response gets called.


https://gerrit.asterisk.org/c/asterisk/+/19460/comment/6e1310b0_d66204ec 
PS2, Line 569: 	.support_level = AST_MODULE_SUPPORT_CORE,
> extended
Done



-- 
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: 3
Gerrit-Owner: Michael Kuron <m.kuron at gmx.de>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Sat, 29 Oct 2022 13:54:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20221029/83e5a80e/attachment.html>