<p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/9505">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/3/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/include/asterisk/res_pjsip.h@424">Patch Set #3, Line 424:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! Refresh token to use for OAuth authentication */<br> AST_STRING_FIELD(refresh_token);<br> /*! Client ID to use for OAuth authentication */<br> AST_STRING_FIELD(oauth_clientid);<br> /*! Secret to use for OAuth authentication */<br> AST_STRING_FIELD(oauth_secret);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This will need to also have an alembic script added for realtime.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip.c">File res/res_pjsip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip.c@2811">Patch Set #3, Line 2811:</a> <code style="font-family:monospace,monospace">static transport_from_endpoint_callback transport_from_endpoint_override_callback;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Needs further thought, though.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c">File res/res_pjsip_outbound_registration.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@354">Patch Set #3, Line 354:</a> <code style="font-family:monospace,monospace">static pj_pool_t *reg_pool;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@590">Patch Set #3, Line 590:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">{<br> RAII_VAR(struct stateless_send_resolver_callback_data *, data, token, ast_free);<br> pjsip_tpselector orig_selector = { .type = PJSIP_TPSELECTOR_NONE, };<br><br> struct sip_outbound_registration_client_state *client_state = data->client_state;<br> pjsip_tx_data *tdata = data->tdata;<br><br> if (status != PJ_SUCCESS) {<br> ast_log(LOG_ERROR, "Resolver failed. Cannot send message\n");<br> return;<br> }<br><br> ast_sip_set_tpselector_from_transport_name(client_state->transport_name, &orig_selector);<br><br> if (orig_selector.type != PJSIP_TPSELECTOR_LISTENER)<br> {<br> ast_log(LOG_ERROR, "transport_reuse requires setting a transport\n");<br> status = PJ_EUNKNOWN;<br> return;<br> }<br><br> /* Copy server addresses */<br> if (addr && addr != &tdata->dest_info.addr) {<br> pj_memcpy( &tdata->dest_info.addr, addr, sizeof(pjsip_server_addresses));<br> }<br><br> if (orig_selector.u.listener->create_transport2) {<br> orig_selector.u.listener->create_transport2(orig_selector.u.listener,<br> pjsip_endpt_get_tpmgr(ast_sip_get_pjsip_endpoint()),<br> ast_sip_get_pjsip_endpoint(),<br> &tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr,<br> tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr_len,<br> tdata,<br> &client_state->transport);<br> } else {<br> orig_selector.u.listener->create_transport(orig_selector.u.listener,<br> pjsip_endpt_get_tpmgr(ast_sip_get_pjsip_endpoint()),<br> ast_sip_get_pjsip_endpoint(),<br> &tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr,<br> tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr_len,<br> &client_state->transport);<br> }<br><br> ast_log(LOG_DEBUG, "Registration using newly created transport %p\n", (void*)client_state->transport);<br> send_on_transport(client_state, tdata);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@660">Patch Set #3, Line 660:</a> <code style="font-family:monospace,monospace"> data = ast_malloc(sizeof(struct stateless_send_resolver_callback_data));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This can fail and go kaboom.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/res/res_pjsip_outbound_registration.c@2593">Patch Set #3, Line 2593:</a> <code style="font-family:monospace,monospace">}</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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)?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0110-oauth.patch">File third-party/pjproject/patches/0110-oauth.patch:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0110-oauth.patch@70">Patch Set #3, Line 70:</a> <code style="font-family:monospace,monospace">+ else //if (pj_stricmp(&c->scheme, &pjsip_DIGEST_STR)==0)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this still be commented out or just removed?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0120-contact-params.patch">File third-party/pjproject/patches/0120-contact-params.patch:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/3/third-party/pjproject/patches/0120-contact-params.patch@64">Patch Set #3, Line 64:</a> <code style="font-family:monospace,monospace">+ const pjsip_param *params )</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9505">change 9505</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9505"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Id214c2d1c550a41fcf564b7df8f3da7be565bd58 </div>
<div style="display:none"> Gerrit-Change-Number: 9505 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Nick French <naf@ou.edu> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Nick French <naf@ou.edu> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 25 Jul 2018 17:27:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>