[Asterisk-code-review] res_pjsip_diversion: implement support for History-Info (asterisk[16])

[Joshua Colp]

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

Change subject: res_pjsip_diversion: implement support for History-Info
......................................................................


Patch Set 11: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/c/asterisk/+/14729/11/include/asterisk/res_pjsip.h 
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/c/asterisk/+/14729/11/include/asterisk/res_pjsip.h@581 
PS11, Line 581: 	/*! Do we add History-Info headers to applicable outgoing requests/responses? */
This should come at the end to preserve backwards ABI.


https://gerrit.asterisk.org/c/asterisk/+/14729/11/res/res_pjsip_diversion.c 
File res/res_pjsip_diversion.c:

https://gerrit.asterisk.org/c/asterisk/+/14729/11/res/res_pjsip_diversion.c@222 
PS11, Line 222: 		fromto_hdr =  pjsip_parse_hdr(rdata->tp_info.pool, &from_name, value.ptr,
This assumes this won't fail, but what if the contents can not be parsed as a From/To?


https://gerrit.asterisk.org/c/asterisk/+/14729/11/res/res_pjsip_diversion.c@243 
PS11, Line 243: static pjsip_fromto_hdr *get_history_info_header_from(pjsip_rx_data *rdata)
This is a lot of duplicated code, can a common function be used that merely has an argument passed in to define behavior?


https://gerrit.asterisk.org/c/asterisk/+/14729/11/res/res_pjsip_diversion.c@265 
PS11, Line 265: 		fromto_hdr =  pjsip_parse_hdr(rdata->tp_info.pool, &from_name, value.ptr,
Same here


https://gerrit.asterisk.org/c/asterisk/+/14729/11/res/res_pjsip_diversion.c@609 
PS11, Line 609: 	param->name = pj_str("index");
These should follow convention elsewhere and be static const pj_str_t, same goes for the others



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I2296369582d4b295c5ea1e60bec391dd1d318fa6
Gerrit-Change-Number: 14729
Gerrit-PatchSet: 11
Gerrit-Owner: Torrey Searle <tsearle at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 09 Sep 2020 10:36:17 +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/20200909/20b0d27a/attachment.html>