[Asterisk-code-review] res pjsip: Implement additional SIP RFCs for Google Voice tr... (asterisk[master])

[Richard Mudgett]

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

Change subject: res_pjsip: Implement additional SIP RFCs for Google Voice trunk compatability
......................................................................


Patch Set 16: Code-Review-1

(11 comments)

The alembic file is missing to update the database schema for the new enum values and parameters.

https://gerrit.asterisk.org/#/c/9505/16/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/#/c/9505/16/include/asterisk/res_pjsip.h@454
PS16, Line 454: AST_VECTOR(ast_sip_service_route_vector, char *);
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.


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip.c
File res/res_pjsip.c:

https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip.c@3396
PS16, Line 3396: 	if (transport_state->flow) {
               : 		ao2_lock(transport_state);
               : 	}
There are two exit points below where we could leave the transport_state locked.


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip.c@4888
PS16, Line 4888: 		ast_sip_message_apply_transport(sip_endpoint->transport, tdata);
This is currently a noop.  I suppose it should stay for completeness in case something changes later.


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip/config_auth.c
File res/res_pjsip/config_auth.c:

https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip/config_auth.c@118
PS16, Line 118: 		if (ast_strlen_zero(auth->refresh_token) || ast_strlen_zero(auth->oauth_clientid) || ast_strlen_zero(auth->oauth_secret)) {
Wrap long line


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip/config_transport.c
File res/res_pjsip/config_transport.c:

https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip/config_transport.c@959
PS16, Line 959: 	if (!strcasecmp(var->value, "udp")) {
              : 		transport->type = AST_TRANSPORT_UDP;
              : 	} else if (!strcasecmp(var->value, "tcp")) {
              : 		transport->type = AST_TRANSPORT_TCP;
              : 	} else if (!strcasecmp(var->value, "tls")) {
              : 		transport->type = AST_TRANSPORT_TLS;
              : 	} else if (!strcasecmp(var->value, "ws")) {
              : 		transport->type = AST_TRANSPORT_WS;
              : 	} else if (!strcasecmp(var->value, "wss")) {
              : 		transport->type = AST_TRANSPORT_WSS;
              : 	} else if (!strcasecmp(var->value, "flow")) {
              : 		transport->flow = 1;
              : 	} else {
              : 		return -1;
              : 	}
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'.

example:
[template-flow](!)
type=transport
transport=flow

[txport-tcp](template-flow)
transport=tcp


if ('flow') {
   transport->flow = 1;
} else {
   transport->flow = 0;
   if ('udp') {
     transport->type = AST_TRANSPORT_UDP;
   } else if ('tcp') {
   ....
   } else {
      return -1;
   }
}


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c
File res/res_pjsip_outbound_registration.c:

https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@180
PS16, Line 180: 					<synopsis>Enables Outbound support for outbound REGISTER requests.</synopsis>
This could use more explanation.  What is outbound support?


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1416
PS16, Line 1416: 	RAII_VAR(char *, cmd, NULL, ast_free);
This RAII_VAR can be easily eliminated.


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1417
PS16, Line 1417: 	char cBuf[4096] = "";
This zeros the entire 4k buffer.  Probably only need to zero the first char in the buffer.


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1420
PS16, Line 1420: 	RAII_VAR(struct ast_json *, jobj, NULL, ast_json_unref);
This RAII_VAR can be easily eliminated.


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@1444
PS16, Line 1444: 			ast_string_field_set(auth, auth_pass, token);
This is modifying a sorcery object that was retrieved from sorcery!  Very bad!  They are supposed to be immutable!


https://gerrit.asterisk.org/#/c/9505/16/res/res_pjsip_outbound_registration.c@2463
PS16, Line 2463: 	ast_sorcery_object_field_register(ast_sip_get_sorcery(), "registration", "support_outbound", "no", OPT_BOOL_T, 1, FLDSET(struct sip_outbound_registration, support_outbound));
OPT_BOOL_T uses true/false as the output strings not yes/no.  Use OPT_YESNO_T instead.



-- 
To view, visit https://gerrit.asterisk.org/9505
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id214c2d1c550a41fcf564b7df8f3da7be565bd58
Gerrit-Change-Number: 9505
Gerrit-PatchSet: 16
Gerrit-Owner: Nick French <naf at ou.edu>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Michael Kuron <m.kuron at gmx.de>
Gerrit-Reviewer: Michael L. Young <elgueromexicano at gmail.com>
Gerrit-Reviewer: Nick French <naf at ou.edu>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Mon, 24 Sep 2018 21:59:17 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180924/5611953c/attachment.html>