[Asterisk-code-review] res pjsip: Implement additional SIP RFCs for Google Voice tr... (asterisk[master])
Joshua Colp
asteriskteam at digium.com
Wed Jul 25 12:27:04 CDT 2018
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/9505 )
Change subject: res_pjsip: Implement additional SIP RFCs for Google Voice trunk compatability
......................................................................
Patch Set 3: Code-Review-1
(8 comments)
https://gerrit.asterisk.org/#/c/9505/3/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:
https://gerrit.asterisk.org/#/c/9505/3/include/asterisk/res_pjsip.h@424
PS3, Line 424: /*! Refresh token to use for OAuth authentication */
: AST_STRING_FIELD(refresh_token);
: /*! Client ID to use for OAuth authentication */
: AST_STRING_FIELD(oauth_clientid);
: /*! Secret to use for OAuth authentication */
: AST_STRING_FIELD(oauth_secret);
This will need to also have an alembic script added for realtime.
https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip.c
File res/res_pjsip.c:
https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip.c@2811
PS3, Line 2811: static transport_from_endpoint_callback transport_from_endpoint_override_callback;
I need to give further thought on this but I don't like this callback mechanism. It's really just been shoved in here so that outbound registration can have a foot hold on it.
I think what may be easier is the ability to tag connections so they can be explicitly requested in a SIP URI. The request URI can then be updated with that new target address skipping DNS resolution, and transport selection will automatically take care of it (based on the transport technology/IP address/port) going out on the correct one.
Needs further thought, though.
https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c
File res/res_pjsip_outbound_registration.c:
https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@354
PS3, Line 354: static pj_pool_t *reg_pool;
The use of this pool in this file is problematic. A pool doesn't release memory until the pool itself is freed, this means that every time you use it in here it's using more and more memory - in particular every time you get a 200 from Google you're using more memory and it will grow.
https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@590
PS3, Line 590: {
: RAII_VAR(struct stateless_send_resolver_callback_data *, data, token, ast_free);
: pjsip_tpselector orig_selector = { .type = PJSIP_TPSELECTOR_NONE, };
:
: struct sip_outbound_registration_client_state *client_state = data->client_state;
: pjsip_tx_data *tdata = data->tdata;
:
: if (status != PJ_SUCCESS) {
: ast_log(LOG_ERROR, "Resolver failed. Cannot send message\n");
: return;
: }
:
: ast_sip_set_tpselector_from_transport_name(client_state->transport_name, &orig_selector);
:
: if (orig_selector.type != PJSIP_TPSELECTOR_LISTENER)
: {
: ast_log(LOG_ERROR, "transport_reuse requires setting a transport\n");
: status = PJ_EUNKNOWN;
: return;
: }
:
: /* Copy server addresses */
: if (addr && addr != &tdata->dest_info.addr) {
: pj_memcpy( &tdata->dest_info.addr, addr, sizeof(pjsip_server_addresses));
: }
:
: if (orig_selector.u.listener->create_transport2) {
: orig_selector.u.listener->create_transport2(orig_selector.u.listener,
: pjsip_endpt_get_tpmgr(ast_sip_get_pjsip_endpoint()),
: ast_sip_get_pjsip_endpoint(),
: &tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr,
: tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr_len,
: tdata,
: &client_state->transport);
: } else {
: orig_selector.u.listener->create_transport(orig_selector.u.listener,
: pjsip_endpt_get_tpmgr(ast_sip_get_pjsip_endpoint()),
: ast_sip_get_pjsip_endpoint(),
: &tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr,
: tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr_len,
: &client_state->transport);
: }
:
: ast_log(LOG_DEBUG, "Registration using newly created transport %p\n", (void*)client_state->transport);
: send_on_transport(client_state, tdata);
I'm not a huge fan of how this is duplicating some logic and directly accessing things. I need to think more about it, and probably ask Teluu what they think.
Really what is needed is a flag for the transport selection process that regc can use to say that it shouldn't reuse a connection if it doesn't already have one established.
https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@660
PS3, Line 660: data = ast_malloc(sizeof(struct stateless_send_resolver_callback_data));
This can fail and go kaboom.
https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@2593
PS3, Line 2593: }
Any reason this couldn't alter the transport information instead of using the callback? Did you just do the callback method to ensure that other things were taken care of too? (Like OPTIONS)?
https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0110-oauth.patch
File third-party/pjproject/patches/0110-oauth.patch:
https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0110-oauth.patch@70
PS3, Line 70: + else //if (pj_stricmp(&c->scheme, &pjsip_DIGEST_STR)==0)
Should this still be commented out or just removed?
https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0120-contact-params.patch
File third-party/pjproject/patches/0120-contact-params.patch:
https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0120-contact-params.patch@64
PS3, Line 64: + const pjsip_param *params )
This changes the API, which breaks every consumer of this. I don't believe Teluu will be willing to accept it - they generally don't.
--
To view, visit https://gerrit.asterisk.org/9505
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id214c2d1c550a41fcf564b7df8f3da7be565bd58
Gerrit-Change-Number: 9505
Gerrit-PatchSet: 3
Gerrit-Owner: Nick French <naf at ou.edu>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Nick French <naf at ou.edu>
Gerrit-Comment-Date: Wed, 25 Jul 2018 17:27:04 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180725/aaf89027/attachment.html>
More information about the asterisk-code-review
mailing list