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

[George Joseph]

Attention is currently required from: Michael Bradeen, Joshua Colp, Benjamin Keith Ford.
George Joseph 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: Code-Review-1

(11 comments)

Patchset:

PS4: 
It's probably already in the existing code but where do we explicitly prevent sending tel uris?


File configs/samples/pjsip.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/75b86763_855048ab 
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.  However, you might want to explain that if the From header is a tel-uri, the endpoint can only be matched by ip, header, or auth_username.  not username.


File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/b7cfe30d_9b99c9b4 
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.  I get that ast_copy_pj_str doesn't have NULL checking but it probably should.


File res/res_pjsip.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/7255d397_61a33a88 
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.  I get that ast_copy_pj_str doesn't have NULL checking but it probably should.


File res/res_pjsip/pjsip_message_filter.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/ca28790d_8c992c19 
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?


File res/res_pjsip_dialog_info_body_generator.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/deb6f87f_e3b3bbfb 
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.  I think that means an invalid sip uri is going to be created wherever from_domain_sanitized is used below.


File res/res_pjsip_endpoint_identifier_anonymous.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/dbc561a8_f814907c 
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.


File res/res_pjsip_endpoint_identifier_user.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/17bcb46b_fa6d63fc 
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?


https://gerrit.asterisk.org/c/asterisk/+/18892/comment/f0df205c_5138eee6 
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.


File res/res_pjsip_nat.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/1475fd45_a24c8fdc 
PS4, Line 115: 
Just a whitespace change?


File res/res_pjsip_path.c:

https://gerrit.asterisk.org/c/asterisk/+/18892/comment/6d4417db_62473948 
PS4, Line 53: 	domain_name = ast_alloca(uri_hostname->slen + 1);
Same as before.  In this case we're going to attempt to look up a domain alias of "" which will always fail so why bother?



-- 
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: Benjamin Keith Ford <bford at digium.com>
Gerrit-Comment-Date: Fri, 19 Aug 2022 09:52:13 +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/20220819/8d73a7d9/attachment-0001.html>