[Asterisk-code-review] pjsip: new endpoint's options to control Connected Line updates (asterisk[13])

[Richard Mudgett]

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

Change subject: pjsip: new endpoint's options to control Connected Line updates
......................................................................


Patch Set 1:

(5 comments)

Since this is a new feature, testsuite tests are required for the patch to be accepted in released branches.  Otherwise, it can only go into the master branch.

https://wiki.asterisk.org/wiki/display/AST/New+Feature+Guidelines

https://gerrit.asterisk.org/#/c/10480/1/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/#/c/10480/1/include/asterisk/res_pjsip.h@563
PS1, Line 563: 	/*! Do we accept connected line updates from this endpoint? */
             : 	unsigned int trust_connected_line;
             : 	/*! Do we send connected line updates to this endpoint? */
             : 	unsigned int send_connected_line;
This is the correct spot for the master version only.  However, for released versions, adding these parameters here breaks ABI compatibility.  For released versions, these must go at the end of the struct ast_sip_endpoint.


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

https://gerrit.asterisk.org/#/c/10480/1/res/res_pjsip.c@192
PS1, Line 192: 				<configOption name="trust_connected_line" default="yes">
             : 					<synopsis>Accept Connected Line updates from this endpoint</synopsis>
             : 				</configOption>
             : 				<configOption name="send_connected_line" default="yes">
It is best to not specify a default here.  Specifying the default here is ignored and can get out of sync with where it is actually effective.  The default specified with ast_sorcery_object_field_register() is used.


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

https://gerrit.asterisk.org/#/c/10480/1/res/res_pjsip/pjsip_configuration.c@1772
PS1, Line 1772: 	ast_sorcery_object_field_register(sip_sorcery, "endpoint", "trust_connected_line", "yes", OPT_BOOL_T, 1, FLDSET(struct ast_sip_endpoint, id.trust_connected_line));
              : 	ast_sorcery_object_field_register(sip_sorcery, "endpoint", "send_connected_line", "yes", OPT_BOOL_T, 1, FLDSET(struct ast_sip_endpoint, id.send_connected_line));
Since you are using yes/no for the strings you need to use OPT_YESNO_T instead of OPT_BOOL_T.  The only difference between the two is that OPT_YESNO_T writes configuration output using yes/no strings rather than true/false strings.


https://gerrit.asterisk.org/#/c/10480/1/res/res_pjsip_caller_id.c
File res/res_pjsip_caller_id.c:

https://gerrit.asterisk.org/#/c/10480/1/res/res_pjsip_caller_id.c@345
PS1, Line 345: 	if (!session->endpoint->id.trust_connected_line || !session->endpoint->id.trust_inbound) {
guidelines: Long lines need to be wrapped when over 90 columns.


if (!session->endpoint->id.trust_connected_line
    || !session->endpoint->id.trust_inbound) {
}


https://gerrit.asterisk.org/#/c/10480/1/res/res_pjsip_caller_id.c@753
PS1, Line 753: 	if (!session->channel
             : 		|| (!session->endpoint->id.send_connected_line
             : 			&& session->inv_session && session->inv_session->state >= PJSIP_INV_STATE_EARLY)) {
guidelines: wrap long line

if (!session->channel
    || (!session->endpoint->id.send_connected_line
        && session->inv_session
        && session->inv_session->state >= PJSIP_INV_STATE_EARLY)) {
}



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I16af967815efd904597ec2f033337e4333d097cd
Gerrit-Change-Number: 10480
Gerrit-PatchSet: 1
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Tue, 16 Oct 2018 20:55:25 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181016/4401a92b/attachment.html>