[Asterisk-code-review] res pjsip: Symmetric transports (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5156 )

Change subject: res_pjsip:  Symmetric transports
......................................................................


Patch Set 1: Code-Review-1

(15 comments)

https://gerrit.asterisk.org/#/c/5156/1/CHANGES
File CHANGES:

PS1, Line 36:    If set to yes, when a request from a dynamic contact comes in on this
            :    transport, the transport name will be saved and used for subsequent
s/comes in on this transport/comes in/


https://gerrit.asterisk.org/#/c/5156/1/configs/samples/pjsip.conf.sample
File configs/samples/pjsip.conf.sample:

Line 845:                         ; comes in on this transport, the transport name will be
s/comes in on this transport/comes in/


https://gerrit.asterisk.org/#/c/5156/1/res/res_pjsip.c
File res/res_pjsip.c:

Line 1181: 					<synopsis>Use the same transport for outgoing reqyests as incoming ones.</synopsis>
sp reqyests


Line 1184: 							comes in on this transport, the transport name will be
s/comes in on this transport/comes in/


Line 2774: 	pj_str_t name = { "x-ast-txp", 9};
static const pj_str_t

How about x_name?


PS1, Line 2793: 	transport_name = ast_malloc(x_transport->value.slen + 1);
              : 	ast_copy_pj_str(transport_name, &x_transport->value, x_transport->value.slen + 1);
ast_malloc() can fail


Line 2803: 	char *transport_name = NULL;
No need to init to NULL.  Its first use is an initialization assignment.


PS1, Line 3028: 	if (sip_dialog_create_from(dlg->pool, &local_uri, endpoint->fromuser, endpoint->fromdomain, &remote_uri, &selector)) {
              : 		pjsip_dlg_terminate(dlg);
Need to dec the dlg->sess_count we temporarily bumped.


Line 3032: 
extraneous blank line


Line 3133: 	char *transport_name = NULL;
No need to init.  Its first use is an initialization.


Line 3141: 		return NULL;
transport_name is leaked here.


PS1, Line 3339: 	transport_name = ast_sip_get_transport_name(endpoint, pjsip_uri_get_uri(sip_uri));
              : 	ast_sip_set_tpselector_from_transport_name(transport_name, &selector);
transport_name is leaked


https://gerrit.asterisk.org/#/c/5156/1/res/res_pjsip/pjsip_message_ip_updater.c
File res/res_pjsip/pjsip_message_ip_updater.c:

Line 158: 	pj_str_t name = { "x-ast-txp", 9};
static const pj_str_t

How about x_name.


Line 348: 	x_transport->name = pj_strdup3(rdata->tp_info.pool, "x-ast-txp");
The "x-ast-txp" string is used in a lot of places.  Shouldn't there be a define to ensure it is always the same string?


https://gerrit.asterisk.org/#/c/5156/1/res/res_pjsip_pubsub.c
File res/res_pjsip_pubsub.c:

Line 617: 			sub_tree->persistence->contact_uri = ast_strdup(contact_uri);
I think you need to ast_free(sub_tree->persistence->conact_uri) first to avoid leaking.

Wait a minute!  Sorcery objects are immutable.  You cannot change a value like this without potential consequences.  This throws the rest of this function into a bit of a quandary since it does this in many places.  Its a good thing these objects are only stored in AstDB by default.  If it were a memory cache you couldn't get away with this because the actual object itself is stored in the cache.


-- 
To view, visit https://gerrit.asterisk.org/5156
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4ee1f51473da32ca54b877cd158523efcef9655f
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes