[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:

(8 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))) {
> To prevent the channel from being deleted by somebody else. […]
Generally everything in regards to a session is expected to be queued up and executed on that session's taskprocessor in a serialized fashion. This limits the locking and also provides the serialized guarantee so things can be simpler.

This function would need to do so to fit within that threading model, and guarantee that session is not manipulated at the same time.


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;
              : 	}
> Would the following be more appropriate? […]
Yes, except if (!session->channel) is fine


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1919 
PS1, Line 1919: 		ast_debug(3, "Transfer accepted\n");
> Would it be better to remove the debug entirely or change it to ... […]
Keeping debug is fine, but does need the channel like you've done


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1933 
PS1, Line 1933: 				pjsip_evsub_terminate(sub, PJ_TRUE);
> This code was based on the xfer_client_on_evsub_state() function in third-party/pjproject/source/pjs […]
I'd update the comment to make this clear.


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1976 
PS1, Line 1976: 			/* If the subscription is not terminated, terminate it now. */
> Would something like the following be sufficient? […]
Yes


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);
              : 				}
> For clarification, do you want a comment before the status = pjsip_evsub_initiate line?
I believe I was referring to the comment before the xfer_client_on_evsub_state function itself.


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@2011 
PS1, Line 2011: 	xfer_cb.on_evsub_state = &xfer_client_on_evsub_state;
Thinking about this some more - I think this now adds a hard dependency on res_pjsip_pubsub. What happens if that module is not loaded and things here are executed?


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);
> The callback occurs as a result of a received SIP message. […]
What stops the session from going away in that case?



-- 
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: Dan Cropp <dan at amtelco.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: mattf <creslin at digium.com>
Gerrit-Comment-Date: Mon, 13 May 2019 16:37:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at digium.com>
Comment-In-Reply-To: Dan Cropp <dan at amtelco.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190513/8d25ebcf/attachment-0001.html>