Change in osmo-msc[master]: fix regression: fix internal MNCC operation

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Wed May 8 23:40:29 UTC 2019


Neels Hofmeyr has submitted this change and it was merged. ( https://gerrit.osmocom.org/13931 )

Change subject: fix regression: fix internal MNCC operation
......................................................................

fix regression: fix internal MNCC operation

While developing the inter-MSC handover refactoring, I was annoyed by the fact
that mncc_tx_to_cc() receives an MNCC message struct containing a msg_type, as
well as a separate msg_type argument, which may deviate from each other. So, as
a first step I wanted to make sure that all callers send identical values for
both by inserting an OSMO_ASSERT(msg_type == msg->msg_type). Later I was going
to remove the separate msg_type argument.

I then forgot to
- carry on to remove the argument and
- to actually test with internal MNCC (it so happens that all of our ttcn3
  tests also use external MNCC).

As a result, the "large refactoring" patch for inter-MSC Handover breaks
internal MNCC operation.

Fix that: remove the separate msg_type argument and make sure that all callers
of mncc_tx_to_cc() indeed pass the desired msg_type in msg->msg_type, and hence
also remove the odd duality of arguments.

Various functions in mncc_builtin.c also exhibit this separate msg_type
argument, which are all unused and make absolutely no sense. Remove those as
well.

Related: OS#3989
Change-Id: I966ce764796982709ea3312e76988a95257acb8d
---
M include/osmocom/msc/gsm_04_08.h
M src/libmsc/gsm_04_08_cc.c
M src/libmsc/mncc_builtin.c
M src/libmsc/mncc_sock.c
M tests/msc_vlr/msc_vlr_test_call.c
M tests/msc_vlr/msc_vlr_tests.c
6 files changed, 57 insertions(+), 39 deletions(-)

Approvals:
  Neels Hofmeyr: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/msc/gsm_04_08.h b/include/osmocom/msc/gsm_04_08.h
index 47747cb..661da42 100644
--- a/include/osmocom/msc/gsm_04_08.h
+++ b/include/osmocom/msc/gsm_04_08.h
@@ -45,7 +45,7 @@
 int gsm48_send_rr_ass_cmd(struct gsm_lchan *dest_lchan, struct gsm_lchan *lchan, uint8_t power_class);
 int gsm48_send_ho_cmd(struct gsm_lchan *old_lchan, struct gsm_lchan *new_lchan, uint8_t power_command, uint8_t ho_ref);
 
-int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg);
+int mncc_tx_to_cc(struct gsm_network *net, void *arg);
 
 /* convert a ASCII phone number to call-control BCD */
 int encode_bcd_number(uint8_t *bcd_lv, uint8_t max_len,
diff --git a/src/libmsc/gsm_04_08_cc.c b/src/libmsc/gsm_04_08_cc.c
index 0624d56..2869bba 100644
--- a/src/libmsc/gsm_04_08_cc.c
+++ b/src/libmsc/gsm_04_08_cc.c
@@ -2010,11 +2010,10 @@
 }
 
 /* Demux incoming genuine calls to GSM CC from MNCC forwarding for inter-MSC handover */
-int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg)
+int mncc_tx_to_cc(struct gsm_network *net, void *arg)
 {
 	const union mncc_msg *msg = arg;
 	struct mncc_call *mncc_call = NULL;
-	OSMO_ASSERT(msg_type == msg->msg_type);
 
 	if (msg->msg_type == MNCC_SETUP_REQ) {
 		/* Incoming call to forward for inter-MSC Handover? */
diff --git a/src/libmsc/mncc_builtin.c b/src/libmsc/mncc_builtin.c
index 6dd7e35..5754976 100644
--- a/src/libmsc/mncc_builtin.c
+++ b/src/libmsc/mncc_builtin.c
@@ -67,7 +67,7 @@
 }
 
 /* on incoming call, look up database and send setup to remote subscr. */
-static int mncc_setup_ind(struct gsm_call *call, int msg_type,
+static int mncc_setup_ind(struct gsm_call *call,
 			  struct gsm_mncc *setup)
 {
 	struct gsm_mncc mncc;
@@ -116,29 +116,33 @@
 	/* send call proceeding */
 	memset(&mncc, 0, sizeof(struct gsm_mncc));
 	mncc.callref = call->callref;
+	mncc.msg_type = MNCC_CALL_PROC_REQ;
 	DEBUGCC(call, remote, "Accepting call.\n");
-	mncc_tx_to_cc(call->net, MNCC_CALL_PROC_REQ, &mncc);
+	mncc_tx_to_cc(call->net, &mncc);
 
 	/* modify mode */
 	memset(&mncc, 0, sizeof(struct gsm_mncc));
 	mncc.callref = call->callref;
+	mncc.msg_type = MNCC_LCHAN_MODIFY;
 	DEBUGCC(call, remote, "Modify channel mode.\n");
-	mncc_tx_to_cc(call->net, MNCC_LCHAN_MODIFY, &mncc);
+	mncc_tx_to_cc(call->net, &mncc);
 
 	/* send setup to remote */
 //	setup->fields |= MNCC_F_SIGNAL;
 //	setup->signal = GSM48_SIGNAL_DIALTONE;
 	setup->callref = remote->callref;
+	setup->msg_type = MNCC_SETUP_REQ;
 	DEBUGCC(call, remote, "Forwarding SETUP to remote.\n");
-	return mncc_tx_to_cc(remote->net, MNCC_SETUP_REQ, setup);
+	return mncc_tx_to_cc(remote->net, setup);
 
 out_reject:
-	mncc_tx_to_cc(call->net, MNCC_REJ_REQ, &mncc);
+	mncc.msg_type = MNCC_REJ_REQ;
+	mncc_tx_to_cc(call->net, &mncc);
 	free_call(call);
 	return 0;
 }
 
-static int mncc_alert_ind(struct gsm_call *call, int msg_type,
+static int mncc_alert_ind(struct gsm_call *call,
 			  struct gsm_mncc *alert)
 {
 	struct gsm_call *remote;
@@ -147,11 +151,12 @@
 	if (!(remote = get_call_ref(call->remote_ref)))
 		return 0;
 	alert->callref = remote->callref;
+	alert->msg_type = MNCC_ALERT_REQ;
 	DEBUGCC(call, remote, "Forwarding ALERT to remote.\n");
-	return mncc_tx_to_cc(remote->net, MNCC_ALERT_REQ, alert);
+	return mncc_tx_to_cc(remote->net, alert);
 }
 
-static int mncc_notify_ind(struct gsm_call *call, int msg_type,
+static int mncc_notify_ind(struct gsm_call *call,
 			   struct gsm_mncc *notify)
 {
 	struct gsm_call *remote;
@@ -160,11 +165,12 @@
 	if (!(remote = get_call_ref(call->remote_ref)))
 		return 0;
 	notify->callref = remote->callref;
+	notify->msg_type = MNCC_NOTIFY_REQ;
 	DEBUGCC(call, remote, "Forwarding NOTIF to remote.\n");
-	return mncc_tx_to_cc(remote->net, MNCC_NOTIFY_REQ, notify);
+	return mncc_tx_to_cc(remote->net, notify);
 }
 
-static int mncc_setup_cnf(struct gsm_call *call, int msg_type,
+static int mncc_setup_cnf(struct gsm_call *call,
 			  struct gsm_mncc *connect)
 {
 	struct gsm_mncc connect_ack;
@@ -173,26 +179,29 @@
 
 	/* acknowledge connect */
 	memset(&connect_ack, 0, sizeof(struct gsm_mncc));
+	connect_ack.msg_type = MNCC_SETUP_COMPL_REQ;
 	connect_ack.callref = call->callref;
 	DEBUGP(DMNCC, "(call %x) Acknowledge SETUP.\n", call->callref);
-	mncc_tx_to_cc(call->net, MNCC_SETUP_COMPL_REQ, &connect_ack);
+	mncc_tx_to_cc(call->net, &connect_ack);
 
 	/* send connect message to remote */
 	if (!(remote = get_call_ref(call->remote_ref)))
 		return 0;
+	connect->msg_type = MNCC_SETUP_RSP;
 	connect->callref = remote->callref;
 	DEBUGCC(call, remote, "Sending CONNECT to remote.\n");
-	mncc_tx_to_cc(remote->net, MNCC_SETUP_RSP, connect);
+	mncc_tx_to_cc(remote->net, connect);
 
 	/* bridge tch */
+	bridge.msg_type = MNCC_BRIDGE;
 	bridge.callref[0] = call->callref;
 	bridge.callref[1] = call->remote_ref;
 	DEBUGCC(call, remote, "Bridging with remote.\n");
 
-	return mncc_tx_to_cc(call->net, MNCC_BRIDGE, &bridge);
+	return mncc_tx_to_cc(call->net, &bridge);
 }
 
-static int mncc_disc_ind(struct gsm_call *call, int msg_type,
+static int mncc_disc_ind(struct gsm_call *call,
 			 struct gsm_mncc *disc)
 {
 	struct gsm_call *remote;
@@ -200,18 +209,20 @@
 	/* send release */
 	DEBUGP(DMNCC, "(call %x) Releasing call with cause %d\n",
 		call->callref, disc->cause.value);
-	mncc_tx_to_cc(call->net, MNCC_REL_REQ, disc);
+	disc->msg_type = MNCC_REL_REQ;
+	mncc_tx_to_cc(call->net, disc);
 
 	/* send disc to remote */
 	if (!(remote = get_call_ref(call->remote_ref))) {
 		return 0;
 	}
+	disc->msg_type = MNCC_DISC_REQ;
 	disc->callref = remote->callref;
 	DEBUGCC(call, remote, "Disconnecting remote with cause %d\n", disc->cause.value);
-	return mncc_tx_to_cc(remote->net, MNCC_DISC_REQ, disc);
+	return mncc_tx_to_cc(remote->net, disc);
 }
 
-static int mncc_rel_ind(struct gsm_call *call, int msg_type, struct gsm_mncc *rel)
+static int mncc_rel_ind(struct gsm_call *call, struct gsm_mncc *rel)
 {
 	struct gsm_call *remote;
 
@@ -221,6 +232,7 @@
 		return 0;
 	}
 
+	rel->msg_type = MNCC_REL_REQ;
 	rel->callref = remote->callref;
 	DEBUGCC(call, remote, "Releasing remote with cause %d\n", rel->cause.value);
 
@@ -230,12 +242,12 @@
 	 * it and then we will end up with a double free and a crash
 	 */
 	free_call(call);
-	mncc_tx_to_cc(remote->net, MNCC_REL_REQ, rel);
+	mncc_tx_to_cc(remote->net, rel);
 
 	return 0;
 }
 
-static int mncc_rel_cnf(struct gsm_call *call, int msg_type, struct gsm_mncc *rel)
+static int mncc_rel_cnf(struct gsm_call *call, struct gsm_mncc *rel)
 {
 	free_call(call);
 	return 0;
@@ -273,10 +285,11 @@
 			struct gsm_mncc rel;
 
 			memset(&rel, 0, sizeof(struct gsm_mncc));
+			rel.msg_type = MNCC_REL_REQ;
 			rel.callref = callref;
 			mncc_set_cause(&rel, GSM48_CAUSE_LOC_PRN_S_LU,
 				       GSM48_CC_CAUSE_RESOURCE_UNAVAIL);
-			mncc_tx_to_cc(net, MNCC_REL_REQ, &rel);
+			mncc_tx_to_cc(net, &rel);
 			goto out_free;
 		}
 		llist_add_tail(&call->entry, &call_list);
@@ -296,47 +309,51 @@
 
 	switch(msg_type) {
 	case MNCC_SETUP_IND:
-		rc = mncc_setup_ind(call, msg_type, arg);
+		rc = mncc_setup_ind(call, arg);
 		break;
 	case MNCC_SETUP_CNF:
-		rc = mncc_setup_cnf(call, msg_type, arg);
+		rc = mncc_setup_cnf(call, arg);
 		break;
 	case MNCC_SETUP_COMPL_IND:
 		break;
 	case MNCC_CALL_CONF_IND:
 		/* we now need to MODIFY the channel */
-		mncc_tx_to_cc(call->net, MNCC_LCHAN_MODIFY, data);
+		data->msg_type = MNCC_LCHAN_MODIFY;
+		mncc_tx_to_cc(call->net, data);
 		break;
 	case MNCC_ALERT_IND:
-		rc = mncc_alert_ind(call, msg_type, arg);
+		rc = mncc_alert_ind(call, arg);
 		break;
 	case MNCC_NOTIFY_IND:
-		rc = mncc_notify_ind(call, msg_type, arg);
+		rc = mncc_notify_ind(call, arg);
 		break;
 	case MNCC_DISC_IND:
-		rc = mncc_disc_ind(call, msg_type, arg);
+		rc = mncc_disc_ind(call, arg);
 		break;
 	case MNCC_REL_IND:
 	case MNCC_REJ_IND:
-		rc = mncc_rel_ind(call, msg_type, arg);
+		rc = mncc_rel_ind(call, arg);
 		break;
 	case MNCC_REL_CNF:
-		rc = mncc_rel_cnf(call, msg_type, arg);
+		rc = mncc_rel_cnf(call, arg);
 		break;
 	case MNCC_FACILITY_IND:
 		break;
 	case MNCC_START_DTMF_IND:
-		rc = mncc_tx_to_cc(net, MNCC_START_DTMF_REJ, data);
+		data->msg_type = MNCC_START_DTMF_REJ;
+		rc = mncc_tx_to_cc(net, data);
 		break;
 	case MNCC_STOP_DTMF_IND:
-		rc = mncc_tx_to_cc(net, MNCC_STOP_DTMF_RSP, data);
+		data->msg_type = MNCC_STOP_DTMF_RSP;
+		rc = mncc_tx_to_cc(net, data);
 		break;
 	case MNCC_MODIFY_IND:
 		mncc_set_cause(data, GSM48_CAUSE_LOC_PRN_S_LU,
 				GSM48_CC_CAUSE_SERV_OPT_UNIMPL);
 		DEBUGP(DMNCC, "(call %x) Rejecting MODIFY with cause %d\n",
 			call->callref, data->cause.value);
-		rc = mncc_tx_to_cc(net, MNCC_MODIFY_REJ, data);
+		data->msg_type = MNCC_MODIFY_REJ;
+		rc = mncc_tx_to_cc(net, data);
 		break;
 	case MNCC_MODIFY_CNF:
 		break;
@@ -345,14 +362,16 @@
 				GSM48_CC_CAUSE_SERV_OPT_UNIMPL);
 		DEBUGP(DMNCC, "(call %x) Rejecting HOLD with cause %d\n",
 			call->callref, data->cause.value);
-		rc = mncc_tx_to_cc(net, MNCC_HOLD_REJ, data);
+		data->msg_type = MNCC_HOLD_REJ;
+		rc = mncc_tx_to_cc(net, data);
 		break;
 	case MNCC_RETRIEVE_IND:
 		mncc_set_cause(data, GSM48_CAUSE_LOC_PRN_S_LU,
 				GSM48_CC_CAUSE_SERV_OPT_UNIMPL);
 		DEBUGP(DMNCC, "(call %x) Rejecting RETRIEVE with cause %d\n",
 			call->callref, data->cause.value);
-		rc = mncc_tx_to_cc(net, MNCC_RETRIEVE_REJ, data);
+		data->msg_type = MNCC_RETRIEVE_REJ;
+		rc = mncc_tx_to_cc(net, data);
 		break;
 	default:
 		LOGP(DMNCC, LOGL_NOTICE, "(call %x) Message unhandled\n", callref);
diff --git a/src/libmsc/mncc_sock.c b/src/libmsc/mncc_sock.c
index b2739e8..0a4e99b 100644
--- a/src/libmsc/mncc_sock.c
+++ b/src/libmsc/mncc_sock.c
@@ -118,7 +118,7 @@
 
 	rc = mncc_prim_check(mncc_prim, rc);
 	if (rc == 0)
-		rc = mncc_tx_to_cc(state->net, mncc_prim->msg_type, mncc_prim);
+		rc = mncc_tx_to_cc(state->net, mncc_prim);
 
 	/* as we always synchronously process the message in mncc_send() and
 	 * its callbacks, we can free the message here. */
diff --git a/tests/msc_vlr/msc_vlr_test_call.c b/tests/msc_vlr/msc_vlr_test_call.c
index 9ca3bf1..b31239e 100644
--- a/tests/msc_vlr/msc_vlr_test_call.c
+++ b/tests/msc_vlr/msc_vlr_test_call.c
@@ -29,7 +29,7 @@
 static void mncc_sends_to_cc(uint32_t msg_type, struct gsm_mncc *mncc)
 {
 	mncc->msg_type = msg_type;
-	mncc_tx_to_cc(net, msg_type, mncc);
+	mncc_tx_to_cc(net, mncc);
 }
 
 /*
diff --git a/tests/msc_vlr/msc_vlr_tests.c b/tests/msc_vlr/msc_vlr_tests.c
index 9c3ecb7..81ac3f7 100644
--- a/tests/msc_vlr/msc_vlr_tests.c
+++ b/tests/msc_vlr/msc_vlr_tests.c
@@ -680,7 +680,7 @@
 		log("MNCC: callref 0x%x: Call Release triggering %s", mncc->callref,
 		    get_mncc_name(on_call_release_mncc_sends_to_cc_data->msg_type));
 
-		mncc_tx_to_cc(net, on_call_release_mncc_sends_to_cc_data->msg_type,
+		mncc_tx_to_cc(net,
 			      on_call_release_mncc_sends_to_cc_data);
 
 		on_call_release_mncc_sends_to_cc_data = NULL;

-- 
To view, visit https://gerrit.osmocom.org/13931
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I966ce764796982709ea3312e76988a95257acb8d
Gerrit-Change-Number: 13931
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190508/8143676e/attachment.html>


More information about the gerrit-log mailing list