[Asterisk-code-review] res pjsip session: Add ability to accept multiple sdp answers (asterisk[13])

[Richard Mudgett]

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

Change subject: res_pjsip_session:  Add ability to accept multiple sdp answers
......................................................................


Patch Set 9: Code-Review-1

(13 comments)

https://gerrit.asterisk.org/#/c/9200/9/configure.ac
File configure.ac:

https://gerrit.asterisk.org/#/c/9200/9/configure.ac@2346
PS9, Line 2346:       AST_C_COMPILE_CHECK([PJSIP_INV_ACCEPT_MULTIPLE_SDP_ANSWERS], [pjsip_cfg()->endpt.accept_multiple_sdp_answers = 0;], [pjsip.h])
Wouldn't it be simpler to move this line up to just after the PJSIP_TLS_TRANSPORT_PROTO line so you don't have to save and restore the flags again?


https://gerrit.asterisk.org/#/c/9200/9/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/#/c/9200/9/include/asterisk/res_pjsip.h@640
PS9, Line 640: 	/*! Follow forked media with a different To tag */
             : 	unsigned int follow_early_media_fork;
             : 	/*! Accept updated SDPs on non-100rel 18X and 2XX responses with the same To tag */
             : 	unsigned int accept_multiple_sdp_answers;
These will have to go at the end of the ast_sip_endpoint struct to not break ABI.  They can go where they belong here in master though.


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c
File res/res_pjsip.c:

https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1030
PS9, Line 1030: 				<configOption name="follow_early_media_fork" default="yes">
FYI:  Putting the default value here is useless.  The only default value that means anything is in the ast_sorcery_object_field_register() lines.


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1035
PS9, Line 1035: 						To tag on the subsequent response is different that the previous one,
s/different that/different than that of/


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1039
PS9, Line 1039: 						<para>
              : 							This option must also be enabled in the <literal>system</literal>
              : 							section for it to take effect here.
              : 						</para>
Make this a note:

<note><para>...</para></note>


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1050
PS9, Line 1050: 						To tag on the subsequent response is the same as that the previous one,
s/as that/as that of/


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1054
PS9, Line 1054: 						<para>
              : 							This option must also be enabled in the <literal>system</literal>
              : 							section for it to take effect here.
              : 						</para>
Make this a note:

<note><para>...</para></note>


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1632
PS9, Line 1632: 						To tag on the subsequent response is different that the previous one,
s/different that/different than that of/


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1635
PS9, Line 1635: 						<para>
              : 							This option must also be enabled on endpoints that require
              : 							this functionality.
              : 						</para>
Make this a note:

<note><para>...</para></note>


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1646
PS9, Line 1646: 						To tag on the subsequent response is the same as that the previous one,
s/as that/as that of/


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip.c@1649
PS9, Line 1649: 						<para>
              : 							This option must also be enabled on endpoints that require
              : 							this functionality.
              : 						</para>
Make this a note:

<note><para>...</para></note>


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip/config_system.c
File res/res_pjsip/config_system.c:

https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip/config_system.c@107
PS9, Line 107: #ifdef HAVE_PJSIP_INV_ACCEPT_MULTIPLE_SDP_ANSWERS
             : 	pjsip_cfg()->endpt.accept_multiple_sdp_answers = system->accept_multiple_sdp_answers;
             : #else
             : 	ast_log(LOG_WARNING, "follow_early_media_fork_same_tag is not supported in this version of pjproject. Ignoring\n");
             : #endif
This warning is unconditional if the pjproject version does not support the feature.  You need to give the warning when the user tries to enable the option.

if (system>accept_multiple_sdp_answers) {
    ast_log(...);
}


https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip_session.c
File res/res_pjsip_session.c:

https://gerrit.asterisk.org/#/c/9200/9/res/res_pjsip_session.c@3093
PS9, Line 3093: 				bail = 0;
Setting bail here is not needed as it is initialized to this.



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I64d071942b79adb2f0a4e13137389b19404fe3d6
Gerrit-Change-Number: 9200
Gerrit-PatchSet: 9
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 21:14:20 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180622/e5be5d00/attachment-0001.html>