[Asterisk-code-review] chan_pjsip: Transfer failure sip result passed through channel: Trans... (asterisk[16])

[Joshua Colp]

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

Change subject: chan_pjsip: Transfer failure sip result passed through channel: Transfer parameter returns the sip protocol error code control: Redirect support additional transfer protocol parameter app_transfer: TRANSFERSTATUSPROTOCOL channel variable addition
......................................................................


Patch Set 1:

(2 comments)

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > > Patch Set 1:
> > > 
> > > Just a heads up (I haven't reviewed this fully) but this is not suitable for inclusion into 16 or 18 as-is. This changes the ast_transfer API in a breaking fashion.
> > 
> > Thank you Joshua.
> > 
> > I believe I modified everywhere ast_transfer was called to use the additional parameter.  Is it unacceptable to do this?
> > 
> > I believe I could make it work without the additional parameter.  It would require setting the return value to the error. Then, in code that calls ast_transfer, it would need to process the result (checking for anything 300+).  Basically, setting it's process error value and change the original value to AST_SUCCESS/AST_FAILURE.  Would this be a more acceptable approach (provided it's well documented)?
> 
> We don't change APIs in a manner that breaks ABI compatibility in release branches, unless absolutely necessary to fix something like a security issue or other serious issue.
> 
> I don't think changing the return value is acceptable, as while ABI compatible that's still a change to the API and a behavior change.
> 
> A second API call being added is perfectly fine, however, as long as the existing one remains and continues to function.

https://gerrit.asterisk.org/c/asterisk/+/15368/1/include/asterisk/channel.h 
File include/asterisk/channel.h:

https://gerrit.asterisk.org/c/asterisk/+/15368/1/include/asterisk/channel.h@2578 
PS1, Line 2578:  * \param protocol result code for transfer (0 success, else sip error code)
This is supposed to not be protocol specific, so SIP can be mentioned as an example


https://gerrit.asterisk.org/c/asterisk/+/15368/1/main/channel.c 
File main/channel.c:

https://gerrit.asterisk.org/c/asterisk/+/15368/1/main/channel.c@6515 
PS1, Line 6515: 			enum ast_control_transfer *message = fr->data.ptr;
This is a change to ast_control_transfer so that needs to be documented as well. Specifically that the message can contain a protocol specific code, and detail the acceptable range.



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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ia6a94784b4925628af122409cdd733c9f29abfc4
Gerrit-Change-Number: 15368
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Cropp <dan at amtelco.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Fri, 22 Jan 2021 15:46:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210122/05f14832/attachment.html>