[Asterisk-code-review] res fax: Fix latent bug exposed by ASTERISK-24841 changes. (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_fax: Fix latent bug exposed by ASTERISK-24841 changes.
......................................................................


res_fax: Fix latent bug exposed by ASTERISK-24841 changes.

Three fax related tests started failing as a result of changes made for
ASTERISK-24841:
tests/fax/pjsip/gateway_t38_g711
tests/fax/sip/gateway_mix1
tests/fax/sip/gateway_mix3

Historically, ast_channel_make_compatible() did nothing if the channels
were already "compatible" even if they had a sub-optimal translation path
already setup.  With the changes from ASTERISK-24841 this is no longer
true in order to allow the best translation paths to always be picked.  In
res_fax.c:fax_gateway_framehook() code manually setup the channels to go
through slin and then called ast_channel_make_compatible().  With the
previous version of ast_channel_make_compatible() this was always a
no-operation.

* Remove call to ast_channel_make_compatible() in fax_gateway_framehook()
that now undoes what was just setup when the framehook is attached.

* Fixed locking around saving the channel formats in
fax_gateway_framehook() to ensure that the formats that are saved are
consistent.

* Fix copy pasta errors in fax_gateway_framehook() that confuses read and
write when dealing with saved channel formats.

ASTERISK-24841
Reported by: Matt Jordan

Change-Id: I6fda0877104a370af586a5e8cf9e161a484da78d
---
M res/res_fax.c
1 file changed, 9 insertions(+), 7 deletions(-)

Approvals:
  Matt Jordan: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Verified



diff --git a/res/res_fax.c b/res/res_fax.c
index c57f446..39cb3b3 100644
--- a/res/res_fax.c
+++ b/res/res_fax.c
@@ -3291,13 +3291,13 @@
 
 		if (gateway->bridged) {
 			ast_set_read_format(chan, gateway->chan_read_format);
-			ast_set_read_format(chan, gateway->chan_write_format);
+			ast_set_write_format(chan, gateway->chan_write_format);
 
 			ast_channel_unlock(chan);
 			peer = ast_channel_bridge_peer(chan);
 			if (peer) {
 				ast_set_read_format(peer, gateway->peer_read_format);
-				ast_set_read_format(peer, gateway->peer_write_format);
+				ast_set_write_format(peer, gateway->peer_write_format);
 				ast_channel_make_compatible(chan, peer);
 			}
 			ast_channel_lock(chan);
@@ -3340,23 +3340,25 @@
 			gateway->timeout_start = ast_tvnow();
 		}
 
+		ast_channel_unlock(chan);
+		ast_channel_lock_both(chan, peer);
+
 		/* we are bridged, change r/w formats to SLIN for v21 preamble
 		 * detection and T.30 */
 		ao2_replace(gateway->chan_read_format, ast_channel_readformat(chan));
-		ao2_replace(gateway->chan_write_format, ast_channel_readformat(chan));
+		ao2_replace(gateway->chan_write_format, ast_channel_writeformat(chan));
 
 		ao2_replace(gateway->peer_read_format, ast_channel_readformat(peer));
-		ao2_replace(gateway->peer_write_format, ast_channel_readformat(peer));
+		ao2_replace(gateway->peer_write_format, ast_channel_writeformat(peer));
 
 		ast_set_read_format(chan, ast_format_slin);
 		ast_set_write_format(chan, ast_format_slin);
 
-		ast_channel_unlock(chan);
 		ast_set_read_format(peer, ast_format_slin);
 		ast_set_write_format(peer, ast_format_slin);
 
-		ast_channel_make_compatible(chan, peer);
-		ast_channel_lock(chan);
+		ast_channel_unlock(peer);
+
 		gateway->bridged = 1;
 	}
 

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6fda0877104a370af586a5e8cf9e161a484da78d
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>