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