[Asterisk-code-review] chan sip.c: T.38 reinvites get dropped by 488 messages (asterisk[11])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: chan_sip.c: T.38 reinvites get dropped by 488 messages
......................................................................


Patch Set 2: Code-Review-1

(7 comments)

I don't think this is the right change to make to solve this problem as it essentially undoes the way T.38 negotiation currently works.

If you could attach the console log to the ASTERISK issue with debug we can see what the right solution is.

https://gerrit.asterisk.org/#/c/3007/2//COMMIT_MSG
Commit Message:

Line 8: 
This commit message doesn't explain what the problem really is or what the solution is.


Line 9: This patch will fix issue reported as ASTERISK-26100.
This is an incorrect format. It should be:

ASTERISK-26100 #close


https://gerrit.asterisk.org/#/c/3007/2/channels/chan_sip.c
File channels/chan_sip.c:

Line 5639: 	ast_debug(2, "T38_PEER_REINVITE: %u, T38_ENABLED: %u, T38_REJECTED: %u, T38_DISABLED: %u, T38_LOCAL_REINVITE: %u\n", T38_PEER_REINVITE, T38_ENABLED, T38_REJECTED, T38_DISABLED, T38_LOCAL_REINVITE);
This would be more useful as a function to convert into a human readable string for the T38 state changed debug message.


Line 7423: 			if(!p->t38id_accepted) transmit_response_with_t38_sdp(p, "200 OK", &p->initreq, XMIT_CRITICAL);
I don't see why adding another field to the structure is needed.


Line 10631: 			ast_verbose("Peer T.38 UDPTL is at port %s\n", ast_sockaddr_stringify(isa));
Already a debug message.


Line 13297: 		ast_verbose("T.38 UDPTL is at %s port %d\n", ast_sockaddr_stringify_addr(&p->ourip), ast_sockaddr_port(&udptladdr));
This is already a debug message.


Line 26115: 				transmit_response_with_t38_sdp(p, "200 OK", &p->initreq, (reinvite ? XMIT_RELIABLE : (req->ignore ?  XMIT_UNRELIABLE : XMIT_CRITICAL)));
This will break negotiation. You can't immediately accept the T.38 reinvite because there may not be anything handling this channel to actually deal with the T.38 traffic. As well you don't have any negotiation information from the other side if there is something.

The code works by queueing a frame which is read by what is handling the channel, it is then up to what is handling it to pass a frame back which accepts the offer. If nothing accepts the offer, then it is terminated.

Your change does away with this completely.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib118fd35b46196943e2afda28f438f063b95ac1c
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Parantido Julius De Rica <parantido at techfusion.it>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes