[Asterisk-code-review] res_pjsip_parameters: Add custom parameter support. (asterisk[master])

[George Joseph]

Attention is currently required from: N A, Joshua Colp, Alexei Gradinari, Stanislav Abramenkov.

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18830 )

Change subject: res_pjsip_parameters: Add custom parameter support.
......................................................................


Patch Set 9: Code-Review-1

(5 comments)

Patchset:

PS9: 
Please rename the function to PJSIP_HEADER_PARAMS to be a bit more clear that it's not for setting core PJSIP parameters.

Move the code into res_pjsip_header_funcs.

At the time this function would be called, have we already finalized the contents of the From header?  I.E. has callerid been set along with any privacy requirements, has "from_domain" and "from_user" from the endpoint been applied?  Does order matter when calling this function along with the CALLERID function?


File res/res_pjsip_parameters.c:

https://gerrit.asterisk.org/c/asterisk/+/18830/comment/b5c1f2c7_ee40496a 
PS9, Line 59: 			<parameter name="header_name" required="false">
            : 				<para>Header in which parameter should be read or set.</para>
            : 				<para>Currently, the only supported header is <literal>From</literal>.</para>
            : 			</parameter>
Since you're implying that future headers may be supported, you should change this to "required".  You should also re-order the parameters to be PJSIP_PARAM(header_name, parameter_type, parameter_name) and adjust the examples below.


https://gerrit.asterisk.org/c/asterisk/+/18830/comment/aeaa4ec9_80287862 
PS9, Line 89: static const struct ast_datastore_info param_datastore = {
            : 	.type = "param_datastore",
            : };
            : 
Not seeing where this is used anymore.


https://gerrit.asterisk.org/c/asterisk/+/18830/comment/9f0d3799_a178b8b7 
PS9, Line 172: 	dlg_info_uri = pjsip_uri_get_uri(dlg_info_name_addr);
Are you absolutely sure this will be a sip/sips uri?


https://gerrit.asterisk.org/c/asterisk/+/18830/comment/614546b4_e0d21108 
PS9, Line 187: pjsip_param_add
I know it's just a macro but can you rename this so it doesn't start with "pjsip_".  In a debug situation, macro names can show up in coredumps.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ifb1bc3c512ad5f6faeaebd7817f004a2ecbd6428
Gerrit-Change-Number: 18830
Gerrit-PatchSet: 9
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-CC: Stanislav Abramenkov <stas.abramenkov at gmail.com>
Gerrit-Attention: N A <asterisk at phreaknet.org>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Attention: Stanislav Abramenkov <stas.abramenkov at gmail.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 13:15:09 +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/20221109/60d74671/attachment-0001.html>