[2/2] Don't connect channels of incompatible voice codec

Submitted by msuraev@sysmocom.de on March 23, 2016, 6:14 p.m.

Details

Message ID 1458756891-28335-2-git-send-email-msuraev@sysmocom.de
State Under Review, archived
Series "Series without cover letter"
Delegated to: zecke
Headers show

Commit Message

msuraev@sysmocom.de March 23, 2016, 6:14 p.m.
From: Max <msuraev@sysmocom.de>

Note: ideally this situation should not happen - we should check
channel compatibility before paging 2nd leg of the call.

Fixes: OS#1663
---
 openbsc/src/libmsc/gsm_04_08.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c
index 95dd647..2f03f75 100644
--- a/openbsc/src/libmsc/gsm_04_08.c
+++ b/openbsc/src/libmsc/gsm_04_08.c
@@ -1614,6 +1614,13 @@  static int tch_map(struct gsm_lchan *lchan, struct gsm_lchan *remote_lchan)
 		return -EINVAL;
 	}
 
+	if (lt != rt) {
+		LOGP(DCC, LOGL_ERROR, "Cannot patch through call with different"
+		     " channel types: local = %s, remote = %s\n",
+		     osmo_gsm48_chan_type2str(lt), osmo_gsm48_chan_type2str(rt));
+		return -EBADSLT;
+	}
+
 	// todo: map between different bts types
 	switch (bts->type) {
 	case GSM_BTS_TYPE_NANOBTS:
@@ -1851,6 +1858,26 @@  static void gsm48_cc_timeout(void *arg)
 
 }
 
+static inline void disconnect_bridge(struct gsm_network *net,
+				    struct gsm_mncc_bridge *bridge)
+{
+	struct gsm_trans *trans0 = trans_find_by_callref(net, bridge->callref[0]);
+	struct gsm_trans *trans1 = trans_find_by_callref(net, bridge->callref[1]);
+	struct gsm_mncc mx_rel;
+	if (!trans0 || !trans1)
+		return;
+
+	memset(&mx_rel, 0, sizeof(struct gsm_mncc));
+	mncc_set_cause(&mx_rel, GSM48_CAUSE_LOC_INN_NET,
+		       GSM48_CC_CAUSE_CHAN_UNACCEPT);
+
+	mx_rel.callref = trans0->callref;
+	gsm48_cc_tx_disconnect(trans0, &mx_rel);
+
+	mx_rel.callref = trans1->callref;
+	gsm48_cc_tx_disconnect(trans1, &mx_rel);
+}
+
 static void gsm48_start_cc_timer(struct gsm_trans *trans, int current,
 				 int sec, int micro)
 {
@@ -3220,7 +3247,12 @@  int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg)
 	/* handle special messages */
 	switch(msg_type) {
 	case MNCC_BRIDGE:
-		return tch_bridge(net, arg);
+		rc = tch_bridge(net, arg);
+		if (rc < 0) {
+			DEBUGP(DCC, "Failed to bridge TCH: %s\n", strerror(-rc));
+			disconnect_bridge(net, arg);
+		}
+		return rc;
 	case MNCC_FRAME_DROP:
 		return tch_recv_mncc(net, data->callref, 0);
 	case MNCC_FRAME_RECV:

Comments

Holger Freyther March 23, 2016, 7:05 p.m.
> On 23 Mar 2016, at 19:14, msuraev@sysmocom.de wrote:
> 
> From: Max <msuraev@sysmocom.de>
> 
> Note: ideally this situation should not happen - we should check
> channel compatibility before paging 2nd leg of the call.
> 
> 
> 
> +	if (lt != rt) {


lt / rt is not declared in this patch (and yes, it belongs into this one. But channel type is still not good enough. Let's assume I have a TCH/H and a TCH/F and I use AMR5.9 on both of these channels. Then the voice codec (and its parameters) are compatible.

I don't know the specific ticket and ultimate goal but I think either you check for codec compatibility or change the wording to refer only to channel type and leave the actual voice codec incompat for another day.





> +		LOGP(DCC, LOGL_ERROR, "Cannot patch through call with different"
> +		     " channel types: local = %s, remote = %s\n",
> +		     osmo_gsm48_chan_type2str(lt), osmo_gsm48_chan_type2str(rt));
> +		return -EBADSLT;
> +	}
> +
> 	// todo: map between different bts types
> 	switch (bts->type) {
> 	case GSM_BTS_TYPE_NANOBTS:
> @@ -1851,6 +1858,26 @@ static void gsm48_cc_timeout(void *arg)
> 
> }
> 
> +static inline void disconnect_bridge(struct gsm_network *net,
> +				    struct gsm_mncc_bridge *bridge)
> +{
> +	struct gsm_trans *trans0 = trans_find_by_callref(net, bridge->callref[0]);
> +	struct gsm_trans *trans1 = trans_find_by_callref(net, bridge->callref[1]);
> +	struct gsm_mncc mx_rel;
> +	if (!trans0 || !trans1)
> +		return;


Can you please elaborate about the intention of this method? You try to undo a failed mapping and do this by disconnecting the call? Is that the right thing to do? Has there been any side effect by the call of tch_map?

In the long run should there be a MNCC_BRIDGE_REJ answer to the MNCC_BRIDGE call?


> @@ -3220,7 +3247,12 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg)
> 	/* handle special messages */
> 	switch(msg_type) {
> 	case MNCC_BRIDGE:
> -		return tch_bridge(net, arg);
> +		rc = tch_bridge(net, arg);
> +		if (rc < 0) {
> +			DEBUGP(DCC, "Failed to bridge TCH: %s\n", strerror(-rc));

Do you think it makes sense to include both callrefs?

holger
msuraev@sysmocom.de March 24, 2016, 11:14 a.m.
Comments are inline.

On 03/23/2016 08:05 PM, Holger Freyther wrote:
>
>
> Can you please elaborate about the intention of this method? You try to undo a failed mapping and do this by disconnecting the call? Is that the right thing to do? Has there been any side effect by the call of tch_map?

Right now the call with incompatible channels is patched through and we
hear broken audio. This method instead disconnects the call for both
subscribers as bug suggested.

> In the long run should there be a MNCC_BRIDGE_REJ answer to the MNCC_BRIDGE call?

I don't think it's worth changing the protocol - this situation only
happens with internal MNCC handler on particular configuration (mixed
TCH/F and H) because we do not support transcoding. We can assume that
all the external MNCC handlers can hadnle it just fine - otherwise
there's no point in using them. We should document that mixing F and H
channels is discouraged when no external MNCC handler is available.