[Asterisk-code-review] res_pjsip: Add TEL URI support for basic calls. (asterisk[16])

Wed Aug 24 10:29:09 CDT 2022

Attention is currently required from: Michael Bradeen, Joshua Colp, George Joseph.
Benjamin Keith Ford has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18892 )

Change subject: res_pjsip: Add TEL URI support for basic calls.
......................................................................

Patch Set 4:

(10 comments)

File configs/samples/pjsip.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/4b8fc2af_89cb5ae5 
PS4, Line 1511: ; You can match a TEL URI to an endpoint using a config object of type
              : ; "identify". Set the endpoint you want it to be associated with, and match
              : ; based on IP. For example:
              : ;
              : ; [tel-uri-identify]
              : ; type=identify
              : ; endpoint=my-endpoint
              : ; match=127.0.0.1:5061
> This is a bit "strange" since match-by-ip applies to anything whether it has a tel uri or not. […]
Done

File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/c7a21817_48890bb3 
PS4, Line 3501: User string or empty string if not present
> Do we really need to return an empty pj_str on error or can we just return NULL. […]
It could go either way. Having it return an empty string could be nice for situations where we don't care what is returned (empty or an actual string). Just removes the need to do a NULL check for that specific case.

File res/res_pjsip.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/dfc2ede9_3e93df62 
PS4, Line 2372: const pj_str_t *ast_sip_pjsip_uri_get_username(pjsip_uri *uri)
> Do we really need to return an empty pj_str on error or can we just return NULL. […]
Ack

File res/res_pjsip/pjsip_message_filter.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/dafd2502_f4182056 
PS4, Line 491: !ast_sip_is_uri_sip_sips(rdata->msg_info.msg->line.req.uri)
             : 		&& 
> I don't think you need this any more do you?  Just test for TEL?
We still want to make sure it's SIP, SIPS, or TEL and nothing else. I think this is still needed, but I could be wrong.

File res/res_pjsip_dialog_info_body_generator.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/687aaa5b_1c988cb7 
PS4, Line 193: 			ast_copy_pj_str(dlg_remote_uri, ast_sip_pjsip_uri_get_hostname(dlg->local.info->uri), sizeof(dlg_remote_uri));
> So dlg_remote-uri is going to be empty if this is a tel-uri. […]
Maybe it would be better to bypass this conditional if it's a TEL URI?

File res/res_pjsip_endpoint_identifier_anonymous.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/ed8775b4_83c12f11 
PS4, Line 39: 	ast_copy_pj_str(domain, ast_sip_pjsip_uri_get_hostname(from), domain_size);
> Same as before.  For a tel-uri the domain will be empty which will produce invalid uris below.
Ack

File res/res_pjsip_endpoint_identifier_user.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/deb97101_9d791df8 
PS4, Line 40: 	ast_copy_pj_str(username, ast_sip_pjsip_uri_get_username(from), username_size);
> I thought we weren't going to allow an identify-by username for a tel uri?
Ah yeah, can change the scheme check back.

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/6bf40ee5_cc696ae1 
PS4, Line 41: 	ast_copy_pj_str(domain, ast_sip_pjsip_uri_get_hostname(from), domain_size);
> Same as before.  An empty domain will produce invalid uris below.
Ack

File res/res_pjsip_nat.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/1e87ee19_4247f64e 
PS4, Line 115: 
> Just a whitespace change?
I originally had a comment there for myself and removed it when cleaning up the patch. Guess it cleaned up some whitespace without me realizing it :)

File res/res_pjsip_path.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/7d07ae00_3329400d 
PS4, Line 53: 	domain_name = ast_alloca(uri_hostname->slen + 1);
> Same as before. […]
Maybe a URI scheme check is needed?

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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: If5729e6cd583be7acf666373bf9f1b9d653ec29a
Gerrit-Change-Number: 18892
Gerrit-PatchSet: 4
Gerrit-Owner: 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: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-CC: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Attention: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Wed, 24 Aug 2022 15:29:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220824/14c8933b/attachment-0001.html>