[Asterisk-code-review] chan_pjsip: Transmit REFER waits for the REFER result setting TRANSF... (...asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11216 )

Change subject: chan_pjsip:  Transmit REFER waits for the REFER result setting TRANSFERSTATUS
......................................................................


Patch Set 1: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c 
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1909 
PS1, Line 1909: 	if (!(session->channel) || !(chan = ast_channel_ref(session->channel))) {
Why do you bump the reference count here on the channel and release it later?


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1906 
PS1, Line 1906: 	if (!(session = (struct ast_sip_session *)pjsip_evsub_get_mod_data(sub, refer_callback_module.id))) {
              : 		return;
              : 	}
              : 	if (!(session->channel) || !(chan = ast_channel_ref(session->channel))) {
              : 		return;
              : 	}
These days we do the assignment and checks separately, minor detail though.


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1919 
PS1, Line 1919: 		ast_debug(3, "Transfer accepted\n");
This doesn't really provide any information or details about which transfer was accepted.


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1933 
PS1, Line 1933: 				pjsip_evsub_terminate(sub, PJ_TRUE);
Will this not happen automatically?


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1970 
PS1, Line 1970: 			/* If the subscription has terminated, return AST_TRANSFER_SUCCESS for 2XX. */
A multiline comment is like:

/* Hello
 * How are you
 * I'm well
 */


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1976 
PS1, Line 1976: 			/* If the subscription is not terminated, terminate it now. */
Specify why you terminate the subscription.


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1980 
PS1, Line 1980: 				status = pjsip_evsub_initiate(sub, pjsip_get_subscribe_method(), 0, &tdata);
              : 				if (status == PJ_SUCCESS) {
              : 					pjsip_evsub_send_request(sub, tdata);
              : 				}
I think at the beginning of the function a comment detailing precisely how this works and the interaction between evsub and it would be needed.


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1987 
PS1, Line 1987: 			ast_debug(3, "Transfer completed: %d %.*s (%s)\n", 
This doesn't give any detail about what channel was transferred or anything like that.


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@2020 
PS1, Line 2020: 	pjsip_evsub_set_mod_data(sub, refer_callback_module.id, session);
What is the reference counting like for this? Is it always guaranteed that session will be valid when the callback is invoked?



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: If6c27c757c66f71e8b75e3fe49da53ebe62395dc
Gerrit-Change-Number: 11216
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Cropp <dan at amtelco.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Tue, 16 Apr 2019 12:53:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190416/7b4f1b9b/attachment.html>