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

[N A]

Attention is currently required from: Sean Bright, Joshua Colp, George Joseph.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18830 )

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


Patch Set 1:

(3 comments)

File res/res_pjsip_session.c:

https://gerrit.asterisk.org/c/asterisk/+/18830/comment/d8888b11_753798e2 
PS1, Line 1703: 	/* Set URI (inside the <>, as opposed to header (outside the <>)) parameters, if requested.
              : 	 * This is often necessary for SS7 trunking over SIP. Most carriers require
              : 	 * that added parameters be URI parameters (as opposed to header parameters).
              : 	 *
              : 	 * other_param is how to add custom URI parameters to the header
              : 	 * XXX: header_param is for header parameters, but doesn't seem to work at the moment.
              : 	 *
              : 	 * Especially when dealing with SS7 or TDM elements over SIP,
              : 	 * injecting parameters into the user and/or method parameters
              : 	 * is required.
              : 	 *
              : 	 * For example, if wanted to set the isup-oli parameter to the channel's ANI2 value:
              : 	 * e.g. pjsip_param_add(dlg_pool, &dlg_info_uri->other_param, "isup-oli", ani2);
I can trim this down as suggested.

However, to address your comments:

> Both of these would be "inside the <>". If they are not "inside the <>" then they are not URI parameters, so all of this just serves to confuse.

Some carriers require parameters inside the <> (URI parameters), and some require them outside the <> (header parameters).

Ideally, both should be settable; pjproject doesn't seem to set header parameters at the moment properly so this just adds URI parameter setting support.

> The 'XXX: header_param is...' comment is irrelevant since you are setting URI parameters and not URI headers.

That was a note about the above, that at this time only support for URI parameters is added, not header parameters. (header parameters, not headers, or URI headers)


XXXXX: I'm possibly getting method and header parameter terminology mixed up, I'll have to double check that but that might explain some of the confusion on my part. URI parameter is inside the <> and one of these other 2 is the part after that.


https://gerrit.asterisk.org/c/asterisk/+/18830/comment/8481624b_5759f045 
PS1, Line 1731: 	while ((param_value = strsep(&params, ","))) {
              : 			param_name = strsep(&param_value, "=");
> How do I add a parameter where the value contains a comma or equals sign? For example I want to add: […]
Parameter names wouldn't contain equal signs, so I don't think the strsep on = is an issue.

If the value contained a comma, that could be an issue. I'll have to think about this - do you have a suggestion?

One thought is perhaps require the value to be quoted in that case, as I think there is an ast_strsep function that would handle it properly in that case.


https://gerrit.asterisk.org/c/asterisk/+/18830/comment/1d064c5e_a93bba8d 
PS1, Line 1734: 			pjsip_param_add(dlg_pool, &dlg_info_uri->other_param, param_name, S_OR(param_value, ""));
> When setting the transport, user, method, ttl, maddr, or lr parameters wouldn't it be better to use  […]
Actually I think Asterisk already handles certain things such as the transport parameter and a couple others natively (transport for sure, since I saw that bit somewhere while working on this).

The goal of this is to handle arbitrary custom parameters which neither Asterisk nor pjproject would be expected to be aware of. So the user shouldn't add transport in this manner, but use the native method instead.

So I think we could either:
1) Detect those parameters and set the special fields instead.
2) Detect those parameters and reject setting them, to avoid conflict with the other mechanisms of setting them
3) Leave as is

Generally speaking, this functionality is not for average users, it is more for advanced usage like those interfacing directly with carriers or SS7 networks.



-- 
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: 1
Gerrit-Owner: N A <mail at interlinked.x10host.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-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Mon, 25 Jul 2022 16:05:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean at seanbright.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220725/b68ed46b/attachment-0001.html>