<p style="white-space: pre-wrap; word-wrap: break-word;">The alembic file is missing to update the database schema for the new enum values and parameters.</p><p>Patch set 16:<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>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/16/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/16/include/asterisk/res_pjsip.h@454">Patch Set #16, Line 454:</a> <code style="font-family:monospace,monospace">AST_VECTOR(ast_sip_service_route_vector, char *);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is this not causing compile errors being declared after it is referenced? This vector struct is referenced by struct ast_sip_transport_state above. It should be moved to above the struct ast_sip_transport_state definition.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/16/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/16/res/res_pjsip.c@3396">Patch Set #16, Line 3396:</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;"> if (transport_state->flow) {<br> ao2_lock(transport_state);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">There are two exit points below where we could leave the transport_state locked.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip.c@4888">Patch Set #16, Line 4888:</a> <code style="font-family:monospace,monospace"> ast_sip_message_apply_transport(sip_endpoint->transport, tdata);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is currently a noop. I suppose it should stay for completeness in case something changes later.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip/config_auth.c">File res/res_pjsip/config_auth.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/16/res/res_pjsip/config_auth.c@118">Patch Set #16, Line 118:</a> <code style="font-family:monospace,monospace"> if (ast_strlen_zero(auth->refresh_token) || ast_strlen_zero(auth->oauth_clientid) || ast_strlen_zero(auth->oauth_secret)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Wrap long line</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip/config_transport.c">File res/res_pjsip/config_transport.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/16/res/res_pjsip/config_transport.c@959">Patch Set #16, Line 959:</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;"> if (!strcasecmp(var->value, "udp")) {<br> transport->type = AST_TRANSPORT_UDP;<br> } else if (!strcasecmp(var->value, "tcp")) {<br> transport->type = AST_TRANSPORT_TCP;<br> } else if (!strcasecmp(var->value, "tls")) {<br> transport->type = AST_TRANSPORT_TLS;<br> } else if (!strcasecmp(var->value, "ws")) {<br> transport->type = AST_TRANSPORT_WS;<br> } else if (!strcasecmp(var->value, "wss")) {<br> transport->type = AST_TRANSPORT_WSS;<br> } else if (!strcasecmp(var->value, "flow")) {<br> transport->flow = 1;<br> } else {<br> return -1;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This isn't going to handle someone who is indecisive about the protocol. It won't handle going from 'flow' to something else like 'tcp'.</p><p style="white-space: pre-wrap; word-wrap: break-word;">example:<br>[template-flow](!)<br>type=transport<br>transport=flow</p><p style="white-space: pre-wrap; word-wrap: break-word;">[txport-tcp](template-flow)<br>transport=tcp</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br>if ('flow') {<br> transport->flow = 1;<br>} else {<br> transport->flow = 0;<br> if ('udp') {<br> transport->type = AST_TRANSPORT_UDP;<br> } else if ('tcp') {<br> ....<br> } else {<br> return -1;<br> }<br>}</pre></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9505/16/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/16/res/res_pjsip_outbound_registration.c@180">Patch Set #16, Line 180:</a> <code style="font-family:monospace,monospace"> <synopsis>Enables Outbound support for outbound REGISTER requests.</synopsis></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This could use more explanation. What is outbound support?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1416">Patch Set #16, Line 1416:</a> <code style="font-family:monospace,monospace"> RAII_VAR(char *, cmd, NULL, ast_free);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This RAII_VAR can be easily eliminated.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1417">Patch Set #16, Line 1417:</a> <code style="font-family:monospace,monospace"> char cBuf[4096] = "";</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This zeros the entire 4k buffer. Probably only need to zero the first char in the buffer.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1420">Patch Set #16, Line 1420:</a> <code style="font-family:monospace,monospace"> RAII_VAR(struct ast_json *, jobj, NULL, ast_json_unref);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This RAII_VAR can be easily eliminated.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1444">Patch Set #16, Line 1444:</a> <code style="font-family:monospace,monospace"> ast_string_field_set(auth, auth_pass, token);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is modifying a sorcery object that was retrieved from sorcery! Very bad! They are supposed to be immutable!</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@2463">Patch Set #16, Line 2463:</a> <code style="font-family:monospace,monospace"> ast_sorcery_object_field_register(ast_sip_get_sorcery(), "registration", "support_outbound", "no", OPT_BOOL_T, 1, FLDSET(struct sip_outbound_registration, support_outbound));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">OPT_BOOL_T uses true/false as the output strings not yes/no. Use OPT_YESNO_T instead.</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: 16 </div>
<div style="display:none"> Gerrit-Owner: Nick French <naf@ou.edu> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Michael Kuron <m.kuron@gmx.de> </div>
<div style="display:none"> Gerrit-Reviewer: Michael L. Young <elgueromexicano@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Nick French <naf@ou.edu> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 24 Sep 2018 21:59:17 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>