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

[George Joseph]

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

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


Patch Set 1:

(16 comments)

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

Line 33: res_pjsip
> This change is in the middle of app_voicemail's section.
Done


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/
> s/comes in on this transport/comes in/

I like the extra clarity.


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/
> s/comes in on this transport/comes in/

I like the extra clarity.


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
Done


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

I like the extra clarity.


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


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
Done


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


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.
Done


Line 3032: 
> extraneous blank line
Done


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


Line 3141: 		return NULL;
> transport_name is leaked here.
Done


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
Done


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
Done


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 de
Done


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 av
> 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.

I fixed the leak but the sorcery object has always been updated like this so I'm not going to address it in this review.


-- 
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: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes