[Asterisk-code-review] channels/chan sip: 180 Ringing not sent after 183 Session Pr... (asterisk[13])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: channels/chan_sip: 180 Ringing not sent after 183 Session Progress
......................................................................


Patch Set 1:

> Regarding changing this behaviour. Maybe it should be configurable
 > in some way to restore the old way of doing this per sip user, and
 > at some point change the default value? Like what was done with the
 > pedantic option.
 > 
 > It seems like at least Cisco SPA's (newest firmware) is broken.
 > They do not care about the early media if they receive a 183 and
 > then a 180. It works with progressinband=yes. Asterisk generates
 > inband ringback when this is not provided from the remote end.

My biggest worry is changing default behaviour. Providing (another) configuration option to restore the previous default behaviour doesn't change the fact that a user can upgrade their system and have their system require modifications to not be broken. That's a path that should only be taken in extreme cases, e.g., security vulnerabilities, major design errors, etc. I'm not sure working around a somewhat goofy upstream piece of equipment qualifies - even if it is definitely a hindrance :-)

What if we instead only triggered the new logic if progressinband=never? That is, your code change would be updated to be:

				/* Well, if it's not reasonable, just send in-band */
				if (ast_test_flag(&p->flags[0], SIP_PROG_INBAND) == SIP_PROG_INBAND_NEVER && !ast_test_flag(&p->flags[0], SIP_RINGING)) {
					transmit_provisional_response(p, "180 Ringing", &p->initreq, 0);
					ast_set_flag(&p->flags[0], SIP_RINGING);
					break;
				}


Which could probably be combined with the previous block into:

	case AST_CONTROL_RINGING:
		if (ast_channel_state(ast) == AST_STATE_RING) {
			p->invitestate = INV_EARLY_MEDIA;
			if (ast_test_flag(&p->flags[0], SIP_PROG_INBAND) == SIP_PROG_INBAND_NEVER)) {
				/* Send 180 ringing if out-of-band seems reasonable */
				transmit_provisional_response(p, "180 Ringing", &p->initreq, 0);
				ast_set_flag(&p->flags[0], SIP_RINGING);
				if (ast_test_flag(&p->flags[0], SIP_PROG_INBAND) != SIP_PROG_INBAND_YES)
					break;
			} else {
				/* Well, if it's not reasonable, just send in-band */
			}
		}
		res = -1;
		break;


That is, we no longer check if progress was sent already, we merely check if we should never generate in-band ringing and, if we shouldn't, we send a 180.

With an UPGRADE note, this feels reasonable - if you've configured your system with progressinband=never, then this is just fixing a corner case for you. If you have progressinband=no or progressinband=yes, we haven't changed your behaviour. And we also aren't changing the behaviour of the default case, progressinband=no. And - I think - this will still let you work around your upstream device.

-- 
To view, visit https://gerrit.asterisk.org/1696
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I498fed853128831b80536f8818cd7b60e641f39c
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Morten Tryfoss <morten at tryfoss.no>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Morten Tryfoss <morten at tryfoss.no>
Gerrit-Reviewer: Olle Johansson <oej at edvina.net>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: No