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

[Dan Cropp]

Dan Cropp 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 3:

(6 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))) {
> Generally everything in regards to a session is expected to be queued up and executed on that sessio […]
After doing some more research, this callback function is serialized via the session's serializer.  Therefore, there should be no need for additional locking or additional serialization.  The associated code has been removed.


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;
              : 	}
> Yes, except if (!session->channel) is fine
ast_channel_ref() call has been removed (see my note above)


https://gerrit.asterisk.org/#/c/11216/1/channels/chan_pjsip.c@1933 
PS1, Line 1933: 				pjsip_evsub_terminate(sub, PJ_TRUE);
> I'd update the comment to make this clear.
Updated comment.


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 believe I was referring to the comment before the xfer_client_on_evsub_state function itself.
I beefed up the comment somewhat.


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. […]
You are correct.
Added a dependency to the module's .requires.


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 stops the session from going away in that case?
Addressed in my comments regarding serialization.



-- 
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: 3
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: Wed, 15 May 2019 16:59:17 +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/20190515/847c0fb8/attachment.html>