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

(9 comments)

Added some questions/responses.

If possible can you look and provide some feedback?
Once I know the right direction, I will modify, retest and resubmit.

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?
To prevent the channel from being deleted by somebody else.  Wasn't sure the intricacies of the PJSIP implementation.  I wasn't confident that it was safe to assume the channel would remain while executing the function.
If there is no need to bump the reference count, I will modify.


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.
Would the following be more appropriate?

if (!(session->channel) {
   return;
}
chan = ast_channel_ref(session->channel);
if (!chan) {
   return;
}


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.
Would it be better to remove the debug entirely or change it to ...

ast_debug(3, “Transfer accepted on channel %s\n”, ast_channel_name(chan));


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?
This code was based on the xfer_client_on_evsub_state() function in third-party/pjproject/source/pjsip/src/pjsua-lib/pjsua_call.c, which includes the call to pjsip_evsub_terminate().  I think the justification is that if the subscription is suppressed, the far end will not terminate it, so it will remain active until it times out.  Since we know it isn’t needed, explicitly terminating it seems like a good idea.


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: […]
Thanks, I will fix


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.
Would something like the following be sufficient?

/* If subscription not terminated and subscription is finished (status code >= 200) terminate it */


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 intera […]
For clarification, do you want a comment before the status = pjsip_evsub_initiate line?


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.
Thanks, I will modify.


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 whe […]
The callback occurs as a result of a received SIP message.  While the message is being processed I don’t think the session can be deleted.  The callback checks for a valid session before doing anything else, so I think I’ve taken care of this concern.



-- 
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:20:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190513/f713108a/attachment.html>