<p>George Joseph <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7710">View Change</a></p><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>(12 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c">File res/res_pjsip/pjsip_options.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@184">Patch Set #3, Line 184:</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;">static pj_status_t send_options_response(pjsip_rx_data *rdata, int code)<br>{<br> pjsip_endpoint *endpt = ast_sip_get_pjsip_endpoint();<br> pjsip_dialog *dlg = pjsip_rdata_get_dlg(rdata);<br> pjsip_transaction *trans = pjsip_rdata_get_tsx(rdata);<br> pjsip_tx_data *tdata;<br> const pjsip_hdr *hdr;<br> pj_status_t status;<br><br> /* Make the response object */<br> status = ast_sip_create_response(rdata, code, NULL, &tdata);<br> if (status != PJ_SUCCESS) {<br> ast_log(LOG_ERROR, "Unable to create response (%d)\n", status);<br> return status;<br> }<br><br> /* Add appropriate headers */<br> if ((hdr = pjsip_endpt_get_capability(endpt, PJSIP_H_ACCEPT, NULL))) {<br> pjsip_msg_add_hdr(tdata->msg, (pjsip_hdr*)pjsip_hdr_clone(tdata->pool, hdr));<br> }<br> if ((hdr = pjsip_endpt_get_capability(endpt, PJSIP_H_ALLOW, NULL))) {<br> pjsip_msg_add_hdr(tdata->msg, (pjsip_hdr*)pjsip_hdr_clone(tdata->pool, hdr));<br> }<br> if ((hdr = pjsip_endpt_get_capability(endpt, PJSIP_H_SUPPORTED, NULL))) {<br> pjsip_msg_add_hdr(tdata->msg, (pjsip_hdr*)pjsip_hdr_clone(tdata->pool, hdr));<br> }<br><br> /*<br> * XXX TODO: pjsip doesn't care a lot about either of these headers -<br> * while it provides specific methods to create them, they are defined<br> * to be the standard string header creation. We never did add them<br> * in chan_sip, although RFC 3261 says they SHOULD. Hard coded here.<br> */<br> ast_sip_add_header(tdata, "Accept-Encoding", DEFAULT_ENCODING);<br> ast_sip_add_header(tdata, "Accept-Language", DEFAULT_LANGUAGE);<br><br> if (dlg && trans) {<br> status = pjsip_dlg_send_response(dlg, trans, tdata);<br> } else {<br> struct ast_sip_endpoint *endpoint;<br><br> endpoint = ast_pjsip_rdata_get_endpoint(rdata);<br> status = ast_sip_send_stateful_response(rdata, tdata, endpoint);<br> ao2_cleanup(endpoint);<br> }<br><br> if (status != PJ_SUCCESS) {<br> ast_log(LOG_ERROR, "Unable to send response (%d)\n", status);<br> }<br><br> return status;<br>}<br><br>static pj_bool_t options_on_rx_request(pjsip_rx_data *rdata)<br>{<br> RAII_VAR(struct ast_sip_endpoint *, endpoint, NULL, ao2_cleanup);<br> pjsip_uri *ruri;<br> pjsip_sip_uri *sip_ruri;<br> char exten[AST_MAX_EXTENSION];<br><br> if (pjsip_method_cmp(&rdata->msg_info.msg->line.req.method, &pjsip_options_method)) {<br> return PJ_FALSE;<br> }<br><br> if (!(endpoint = ast_pjsip_rdata_get_endpoint(rdata))) {<br> return PJ_FALSE;<br> }<br><br> ruri = rdata->msg_info.msg->line.req.uri;<br> if (!PJSIP_URI_SCHEME_IS_SIP(ruri) && !PJSIP_URI_SCHEME_IS_SIPS(ruri)) {<br> send_options_response(rdata, 416);<br> return PJ_TRUE;<br> }<br><br> sip_ruri = pjsip_uri_get_uri(ruri);<br> ast_copy_pj_str(exten, &sip_ruri->user, sizeof(exten));<br><br> /*<br> * We may want to match in the dialplan without any user<br> * options getting in the way.<br> */<br> AST_SIP_USER_OPTIONS_TRUNCATE_CHECK(exten);<br><br> if (ast_shutting_down()) {<br> /*<br> * Not taking any new calls at this time.<br> * Likely a server availability OPTIONS poll.<br> */<br> send_options_response(rdata, 503);<br> } else if (!ast_strlen_zero(exten)<br> && !ast_exists_extension(NULL, endpoint->context, exten, 1, NULL)) {<br> send_options_response(rdata, 404);<br> } else {<br> send_options_response(rdata, 200);<br> }<br> return PJ_TRUE;<br>}<br><br>static pjsip_module options_module = {<br> .name = {"Options Module", 14},<br> .id = -1,<br> .priority = PJSIP_MOD_PRIORITY_APPLICATION,<br> .on_rx_request = options_on_rx_request,<br>};<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Given the work being done, it might be a good time to split out inbound OPTIONS handling to its own source file.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@316">Patch Set #3, Line 316:</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;">static int sip_contact_status_hash(const void *obj, const int flags)<br>{<br> const struct ast_sip_contact_status *object;<br> const char *key;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_KEY:<br> key = obj;<br> break;<br> case OBJ_SEARCH_OBJECT:<br> object = obj;<br> key = object->name;<br> break;<br> default:<br> ast_assert(0);<br> return 0;<br> }<br> return ast_str_hash(key);<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Replace with AO2_STRING_FIELD_HASH_FN()</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@336">Patch Set #3, Line 336:</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;">/*! \brief Comparator function for contact statuses */<br>static int sip_contact_status_cmp(void *obj, void *arg, int flags)<br>{<br> const struct ast_sip_contact_status *object_left = obj;<br> const struct ast_sip_contact_status *object_right = arg;<br> const char *right_key = arg;<br> int cmp;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_OBJECT:<br> right_key = object_right->name;<br> /* Fall through */<br> case OBJ_SEARCH_KEY:<br> cmp = strcmp(object_left->name, right_key);<br> break;<br> case OBJ_SEARCH_PARTIAL_KEY:<br> cmp = strncmp(object_left->name, right_key, strlen(right_key));<br> break;<br> default:<br> cmp = 0;<br> break;<br> }<br> if (cmp) {<br> return 0;<br> }<br> return CMP_MATCH;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Replace with AO2_STRING_FIELD_CMP_FN()</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@441">Patch Set #3, Line 441:</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;">/*! \brief Hashing function for OPTIONS AORs */<br>static int sip_options_aor_hash(const void *obj, const int flags)<br>{<br> const struct sip_options_aor *object;<br> const char *key;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_KEY:<br> key = obj;<br> break;<br> case OBJ_SEARCH_OBJECT:<br> object = obj;<br> key = object->name;<br> break;<br> default:<br> ast_assert(0);<br> return 0;<br> }<br> return ast_str_hash(key);<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Replace with AO2_STRING_FIELD_HASH_FN()</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@462">Patch Set #3, Line 462:</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;">/*! \brief Comparator function for SIP OPTIONS AORs */<br>static int sip_options_aor_cmp(void *obj, void *arg, int flags)<br>{<br> const struct sip_options_aor *object_left = obj;<br> const struct sip_options_aor *object_right = arg;<br> const char *right_key = arg;<br> int cmp;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_OBJECT:<br> right_key = object_right->name;<br> /* Fall through */<br> case OBJ_SEARCH_KEY:<br> cmp = strcmp(object_left->name, right_key);<br> break;<br> case OBJ_SEARCH_PARTIAL_KEY:<br> cmp = strncmp(object_left->name, right_key, strlen(right_key));<br> break;<br> default:<br> cmp = 0;<br> break;<br> }<br> if (cmp) {<br> return 0;<br> }<br> return CMP_MATCH;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Replace with AO2_STRING_FIELD_CMP_FN()</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@490">Patch Set #3, Line 490:</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;">/*! \brief Hashing function for endpoint state compositors */<br>static int sip_endpoint_state_compositor_hash(const void *obj, const int flags)<br>{<br> const struct sip_options_endpoint_state_compositor *object;<br> const char *key;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_KEY:<br> key = obj;<br> break;<br> case OBJ_SEARCH_OBJECT:<br> object = obj;<br> key = object->name;<br> break;<br> default:<br> ast_assert(0);<br> return 0;<br> }<br> return ast_str_hash(key);<br>}<br><br>/*! \brief Comparator function for endpoint state compositors */<br>static int sip_endpoint_state_compositor_cmp(void *obj, void *arg, int flags)<br>{<br> const struct sip_options_endpoint_state_compositor *object_left = obj;<br> const struct sip_options_endpoint_state_compositor *object_right = arg;<br> const char *right_key = arg;<br> int cmp;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_OBJECT:<br> right_key = object_right->name;<br> /* Fall through */<br> case OBJ_SEARCH_KEY:<br> cmp = strcmp(object_left->name, right_key);<br> break;<br> case OBJ_SEARCH_PARTIAL_KEY:<br> cmp = strncmp(object_left->name, right_key, strlen(right_key));<br> break;<br> default:<br> cmp = 0;<br> break;<br> }<br> if (cmp) {<br> return 0;<br> }<br> return CMP_MATCH;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yada Yada.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@665">Patch Set #3, Line 665:</a> <code style="font-family:monospace,monospace"> return 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">contact_callback_data leak?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@672">Patch Set #3, Line 672:</a> <code style="font-family:monospace,monospace"> return 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">same</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@1185">Patch Set #3, Line 1185:</a> <code style="font-family:monospace,monospace">""</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you use the "<aor_name>/options", it'll show with a "pjsip show scheduled tasks" rather than "task_00000000".</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@1269">Patch Set #3, Line 1269:</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;">/*! \brief Hashing function for endpoint AOR status */<br>static int sip_options_endpoint_aor_status_hash(const void *obj, const int flags)<br>{<br> const struct sip_options_endpoint_aor_status *object;<br> const char *key;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_KEY:<br> key = obj;<br> break;<br> case OBJ_SEARCH_OBJECT:<br> object = obj;<br> key = object->name;<br> break;<br> default:<br> ast_assert(0);<br> return 0;<br> }<br> return ast_str_hash(key);<br>}<br><br>/*! \brief Comparator function for endpoint AOR status */<br>static int sip_options_endpoint_aor_status_cmp(void *obj, void *arg, int flags)<br>{<br> const struct sip_options_endpoint_aor_status *object_left = obj;<br> const struct sip_options_endpoint_aor_status *object_right = arg;<br> const char *right_key = arg;<br> int cmp;<br><br> switch (flags & OBJ_SEARCH_MASK) {<br> case OBJ_SEARCH_OBJECT:<br> right_key = object_right->name;<br> /* Fall through */<br> case OBJ_SEARCH_KEY:<br> cmp = strcmp(object_left->name, right_key);<br> break;<br> case OBJ_SEARCH_PARTIAL_KEY:<br> cmp = strncmp(object_left->name, right_key, strlen(right_key));<br> break;<br> default:<br> cmp = 0;<br> break;<br> }<br> if (cmp) {<br> return 0;<br> }<br> return CMP_MATCH;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">replace with yada yada...</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@1337">Patch Set #3, Line 1337:</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;"> endpoint_state_compositor->aor_statuses = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_NOLOCK, 0, AOR_STATUS_BUCKETS,<br> sip_options_endpoint_aor_status_hash, NULL, sip_options_endpoint_aor_status_cmp);<br> if (!endpoint_state_compositor->aor_statuses) {<br> ao2_unlock(sip_options_endpoint_state_compositors);<br> ao2_ref(endpoint_state_compositor, -1);<br> return NULL;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does this really need to be a container? In the realistic scenario, it's a 1:1 ratio but even if it's a handful, a vector might be better.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7710/3/res/res_pjsip/pjsip_options.c@1828">Patch Set #3, Line 1828:</a> <code style="font-family:monospace,monospace">""</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"<aor_name>/options"</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7710">change 7710</a>. To unsubscribe, 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/7710"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I6a5ebbfca9001dfe933eaeac4d3babd8d2e6f082 </div>
<div style="display:none"> Gerrit-Change-Number: 7710 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </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: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 13 Feb 2018 15:06:09 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>