diff mbox

[1/2] Refactor internal mncc code

Message ID 1458756891-28335-1-git-send-email-msuraev@sysmocom.de
State Superseded
Headers show

Commit Message

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

Consistently use enums instead of (u)int, char etc.
Use bool for boolean types.
Add extra debug output with channel mode and type to simplify
troubleshooting.
---
 openbsc/include/openbsc/bsc_api.h    |  4 +++-
 openbsc/include/openbsc/gsm_04_08.h  |  2 +-
 openbsc/include/openbsc/mncc.h       |  6 ++++--
 openbsc/include/openbsc/mncc_int.h   |  5 ++++-
 openbsc/src/libbsc/bsc_api.c         | 21 ++++++++++++---------
 openbsc/src/libbsc/gsm_04_08_utils.c | 12 +++---------
 openbsc/src/libmsc/gsm_04_08.c       | 22 +++++++++++++++++-----
 openbsc/src/libmsc/mncc_builtin.c    | 15 +++++++++++----
 openbsc/src/osmo-nitb/bsc_hack.c     |  4 +++-
 9 files changed, 58 insertions(+), 33 deletions(-)

Comments

Holger Freyther March 23, 2016, 6:35 p.m. UTC | #1
> On 23 Mar 2016, at 19:14, msuraev@sysmocom.de wrote:
> 
> From: Max <msuraev@sysmocom.de>
> 
> Consistently use enums instead of (u)int, char etc.
> Use bool for boolean types.
> Add extra debug output with channel mode and type to simplify
> troubleshooting.

Nack.

* You touch an external protocol that is versioned. Does the size of the structure change? Either way it needs to be in the commit messages.

* You are mixing "add debug messages" and "change types". There is no connection between these two logical changes and they should not be in the same commit.
diff mbox

Patch

diff --git a/openbsc/include/openbsc/bsc_api.h b/openbsc/include/openbsc/bsc_api.h
index a3d12f2..d0a0164 100644
--- a/openbsc/include/openbsc/bsc_api.h
+++ b/openbsc/include/openbsc/bsc_api.h
@@ -3,6 +3,8 @@ 
 #ifndef OPENBSC_BSC_API_H
 #define OPENBSC_BSC_API_H
 
+#include <stdbool.h>
+
 #include "gsm_data.h"
 
 #define BSC_API_CONN_POL_ACCEPT	0
@@ -45,7 +47,7 @@  struct bsc_api {
 
 int bsc_api_init(struct gsm_network *network, struct bsc_api *api);
 int gsm0808_submit_dtap(struct gsm_subscriber_connection *conn, struct msgb *msg, int link_id, int allow_sacch);
-int gsm0808_assign_req(struct gsm_subscriber_connection *conn, int chan_mode, int full_rate);
+int gsm0808_assign_req(struct gsm_subscriber_connection *conn, enum gsm48_chan_mode chan_mode, bool full_rate);
 int gsm0808_cipher_mode(struct gsm_subscriber_connection *conn, int cipher,
 			const uint8_t *key, int len, int include_imeisv);
 int gsm0808_page(struct gsm_bts *bts, unsigned int page_group,
diff --git a/openbsc/include/openbsc/gsm_04_08.h b/openbsc/include/openbsc/gsm_04_08.h
index fd0b89d..d333e13 100644
--- a/openbsc/include/openbsc/gsm_04_08.h
+++ b/openbsc/include/openbsc/gsm_04_08.h
@@ -79,7 +79,7 @@  int gsm48_extract_mi(uint8_t *classmark2, int length, char *mi_string, uint8_t *
 int gsm48_paging_extract_mi(struct gsm48_pag_resp *pag, int length, char *mi_string, uint8_t *mi_type);
 int gsm48_handle_paging_resp(struct gsm_subscriber_connection *conn, struct msgb *msg, struct gsm_subscriber *subscr);
 
-int gsm48_lchan_modify(struct gsm_lchan *lchan, uint8_t lchan_mode);
+int gsm48_lchan_modify(struct gsm_lchan *lchan, enum gsm48_chan_mode lchan_mode);
 int gsm48_rx_rr_modif_ack(struct msgb *msg);
 int gsm48_parse_meas_rep(struct gsm_meas_rep *rep, struct msgb *msg);
 
diff --git a/openbsc/include/openbsc/mncc.h b/openbsc/include/openbsc/mncc.h
index 49f0c8b..ce2f87d 100644
--- a/openbsc/include/openbsc/mncc.h
+++ b/openbsc/include/openbsc/mncc.h
@@ -26,6 +26,8 @@ 
 
 #include <osmocom/core/linuxlist.h>
 #include <osmocom/gsm/mncc.h>
+#include <osmocom/gsm/gsm_utils.h>
+#include <osmocom/gsm/protocol/gsm_04_08.h>
 
 #include <stdint.h>
 
@@ -156,8 +158,8 @@  struct gsm_mncc {
 	int		emergency;
 	char		imsi[16];
 
-	unsigned char	lchan_type;
-	unsigned char	lchan_mode;
+	enum gsm_chan_t lchan_type;
+	enum gsm48_chan_mode lchan_mode;
 };
 
 struct gsm_data_frame {
diff --git a/openbsc/include/openbsc/mncc_int.h b/openbsc/include/openbsc/mncc_int.h
index 213ce14..dafef25 100644
--- a/openbsc/include/openbsc/mncc_int.h
+++ b/openbsc/include/openbsc/mncc_int.h
@@ -3,12 +3,15 @@ 
 
 #include <stdint.h>
 
+#include <osmocom/gsm/gsm_utils.h>
+#include <osmocom/gsm/protocol/gsm_04_08.h>
+
 struct mncc_int {
 	uint8_t def_codec[2];
 };
 
 extern struct mncc_int mncc_int;
 
-uint8_t mncc_codec_for_mode(int lchan_type);
+enum gsm48_chan_mode mncc_codec_for_mode(enum gsm_chan_t lchan_type);
 
 #endif
diff --git a/openbsc/src/libbsc/bsc_api.c b/openbsc/src/libbsc/bsc_api.c
index e6d820d..dadb082 100644
--- a/openbsc/src/libbsc/bsc_api.c
+++ b/openbsc/src/libbsc/bsc_api.c
@@ -21,6 +21,8 @@ 
  *
  */
 
+#include <stdbool.h>
+
 #include <openbsc/bsc_api.h>
 #include <openbsc/bsc_rll.h>
 #include <openbsc/gsm_data.h>
@@ -34,7 +36,7 @@ 
 #include <openbsc/trau_mux.h>
 
 #include <osmocom/gsm/protocol/gsm_08_08.h>
-
+#include <osmocom/gsm/protocol/gsm_04_08.h>
 #include <osmocom/core/talloc.h>
 
 #define GSM0808_T10_VALUE    6, 0
@@ -155,7 +157,7 @@  static void assignment_t10_timeout(void *_conn)
  * Handle the multirate config
  */
 static void handle_mr_config(struct gsm_subscriber_connection *conn,
-			     struct gsm_lchan *lchan, int full_rate)
+			     struct gsm_lchan *lchan, bool full_rate)
 {
 	struct bsc_api *api;
 	api = conn->bts->network->bsc_api;
@@ -197,12 +199,10 @@  static void handle_mr_config(struct gsm_subscriber_connection *conn,
  * -> Assignment Complete/Assignment Failure
  *  5.) Release the SDCCH, continue signalling on the new link
  */
-static int handle_new_assignment(struct gsm_subscriber_connection *conn, int chan_mode, int full_rate)
+static int handle_new_assignment(struct gsm_subscriber_connection *conn, enum gsm48_chan_mode chan_mode, bool full_rate)
 {
 	struct gsm_lchan *new_lchan;
-	int chan_type;
-
-	chan_type = full_rate ? GSM_LCHAN_TCH_F : GSM_LCHAN_TCH_H;
+	enum gsm_chan_t chan_type = full_rate ? GSM_LCHAN_TCH_F : GSM_LCHAN_TCH_H;
 
 	new_lchan = lchan_alloc(conn->bts, chan_type, 0);
 
@@ -337,7 +337,7 @@  int gsm0808_submit_dtap(struct gsm_subscriber_connection *conn,
 /*
  * \brief Check if the given channel is compatible with the mode/fullrate
  */
-static int chan_compat_with_mode(struct gsm_lchan *lchan, int chan_mode, int full_rate)
+static int chan_compat_with_mode(struct gsm_lchan *lchan, enum gsm48_chan_mode chan_mode, bool full_rate)
 {
 	switch (chan_mode) {
 	case GSM48_CMODE_SIGN:
@@ -384,7 +384,8 @@  static int chan_compat_with_mode(struct gsm_lchan *lchan, int chan_mode, int ful
  *
  * TODO: Add multirate configuration, make it work for more than audio.
  */
-int gsm0808_assign_req(struct gsm_subscriber_connection *conn, int chan_mode, int full_rate)
+int gsm0808_assign_req(struct gsm_subscriber_connection *conn,
+		       enum gsm48_chan_mode chan_mode, bool full_rate)
 {
 	struct bsc_api *api;
 	api = conn->bts->network->bsc_api;
@@ -394,7 +395,9 @@  int gsm0808_assign_req(struct gsm_subscriber_connection *conn, int chan_mode, in
 			goto error;
 	} else {
 		LOGP(DMSC, LOGL_NOTICE,
-			"Sending ChanModify for speech %d %d\n", chan_mode, full_rate);
+		     "Sending ChanModify for speech: %s on channel %s, "
+		     "full_rate %d\n", osmo_gsm48_chan_mode2str(chan_mode),
+		     osmo_gsm48_chan_type2str(conn->lchan->type), full_rate);
 		if (chan_mode == GSM48_CMODE_SPEECH_AMR)
 			handle_mr_config(conn, conn->lchan, full_rate);
 
diff --git a/openbsc/src/libbsc/gsm_04_08_utils.c b/openbsc/src/libbsc/gsm_04_08_utils.c
index 8c6dbef..79a8547 100644
--- a/openbsc/src/libbsc/gsm_04_08_utils.c
+++ b/openbsc/src/libbsc/gsm_04_08_utils.c
@@ -488,7 +488,7 @@  int gsm48_send_rr_ass_cmd(struct gsm_lchan *dest_lchan, struct gsm_lchan *lchan,
 }
 
 /* 9.1.5 Channel mode modify: Modify the mode on the MS side */
-int gsm48_tx_chan_mode_modify(struct gsm_lchan *lchan, uint8_t mode)
+int gsm48_tx_chan_mode_modify(struct gsm_lchan *lchan, enum gsm48_chan_mode mode)
 {
 	struct msgb *msg = gsm48_msgb_alloc_name("GSM 04.08 CHN MOD");
 	struct gsm48_hdr *gh = (struct gsm48_hdr *) msgb_put(msg, sizeof(*gh));
@@ -513,15 +513,9 @@  int gsm48_tx_chan_mode_modify(struct gsm_lchan *lchan, uint8_t mode)
 	return gsm48_sendmsg(msg);
 }
 
-int gsm48_lchan_modify(struct gsm_lchan *lchan, uint8_t lchan_mode)
+int gsm48_lchan_modify(struct gsm_lchan *lchan, enum gsm48_chan_mode lchan_mode)
 {
-	int rc;
-
-	rc = gsm48_tx_chan_mode_modify(lchan, lchan_mode);
-	if (rc < 0)
-		return rc;
-
-	return rc;
+	return gsm48_tx_chan_mode_modify(lchan, lchan_mode);
 }
 
 int gsm48_rx_rr_modif_ack(struct msgb *msg)
diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c
index 1524ec4..95dd647 100644
--- a/openbsc/src/libmsc/gsm_04_08.c
+++ b/openbsc/src/libmsc/gsm_04_08.c
@@ -59,6 +59,7 @@ 
 #include <osmocom/gsm/gsm48.h>
 #include <osmocom/gsm/gsm0480.h>
 #include <osmocom/gsm/gsm_utils.h>
+#include <osmocom/gsm/protocol/gsm_04_08.h>
 #include <osmocom/core/msgb.h>
 #include <osmocom/core/talloc.h>
 #include <osmocom/gsm/tlv.h>
@@ -1598,11 +1599,15 @@  static int tch_map(struct gsm_lchan *lchan, struct gsm_lchan *remote_lchan)
 {
 	struct gsm_bts *bts = lchan->ts->trx->bts;
 	struct gsm_bts *remote_bts = remote_lchan->ts->trx->bts;
+	enum gsm_chan_t lt = lchan->type, rt = remote_lchan->type;
 	int rc;
 
-	DEBUGP(DCC, "Setting up TCH map between (bts=%u,trx=%u,ts=%u) and (bts=%u,trx=%u,ts=%u)\n",
-		bts->nr, lchan->ts->trx->nr, lchan->ts->nr,
-		remote_bts->nr, remote_lchan->ts->trx->nr, remote_lchan->ts->nr);
+	DEBUGP(DCC, "Setting up TCH map between (bts=%u,trx=%u,ts=%u,%s) and "
+	       "(bts=%u,trx=%u,ts=%u,%s)\n",
+	       bts->nr, lchan->ts->trx->nr, lchan->ts->nr,
+	       osmo_gsm48_chan_type2str(lt),
+	       remote_bts->nr, remote_lchan->ts->trx->nr, remote_lchan->ts->nr,
+	       osmo_gsm48_chan_type2str(rt));
 
 	if (bts->type != remote_bts->type) {
 		LOGP(DCC, LOGL_ERROR, "Cannot switch calls between different BTS types yet\n");
@@ -2999,6 +3004,7 @@  static int tch_rtp_create(struct gsm_network *net, uint32_t callref)
 	struct gsm_bts *bts;
 	struct gsm_lchan *lchan;
 	struct gsm_trans *trans;
+	enum gsm48_chan_mode m;
 
 	/* Find callref */
 	trans = trans_find_by_callref(net, callref);
@@ -3038,8 +3044,11 @@  static int tch_rtp_create(struct gsm_network *net, uint32_t callref)
 	 */
 	if (lchan->tch_mode == GSM48_CMODE_SIGN) {
 		trans->conn->mncc_rtp_create_pending = 1;
-		return gsm0808_assign_req(trans->conn,
-				mncc_codec_for_mode(lchan->type),
+		m = mncc_codec_for_mode(lchan->type);
+		LOGP(DMNCC, LOGL_DEBUG, "RTP create: codec=%s, chan_type=%s\n",
+		     osmo_gsm48_chan_mode2str(m),
+		     osmo_gsm48_chan_type2str(lchan->type));
+		return gsm0808_assign_req(trans->conn, m,
 				lchan->type != GSM_LCHAN_TCH_H);
 	}
 
@@ -3068,6 +3077,9 @@  static int tch_rtp_connect(struct gsm_network *net, void *arg)
 	}
 
 	lchan = trans->conn->lchan;
+	LOGP(DMNCC, LOGL_DEBUG, "RTP connect: codec=%s, chan_type=%s\n",
+		     osmo_gsm48_chan_mode2str(mncc_codec_for_mode(lchan->type)),
+		     osmo_gsm48_chan_type2str(lchan->type));
 
 	/* TODO: Check if payload_msg_type is compatible with what we have */
 	if (rtp->payload_type != lchan->abis_ip.rtp_payload) {
diff --git a/openbsc/src/libmsc/mncc_builtin.c b/openbsc/src/libmsc/mncc_builtin.c
index 77df6fb..b1993ca 100644
--- a/openbsc/src/libmsc/mncc_builtin.c
+++ b/openbsc/src/libmsc/mncc_builtin.c
@@ -27,6 +27,9 @@ 
 #include <string.h>
 #include <errno.h>
 
+#include <osmocom/gsm/gsm_utils.h>
+#include <osmocom/gsm/protocol/gsm_04_08.h>
+
 #include <openbsc/gsm_04_08.h>
 #include <openbsc/debug.h>
 #include <openbsc/mncc.h>
@@ -65,7 +68,7 @@  static struct gsm_call *get_call_ref(uint32_t callref)
 	return NULL;
 }
 
-uint8_t mncc_codec_for_mode(int lchan_type)
+enum gsm48_chan_mode mncc_codec_for_mode(enum gsm_chan_t lchan_type)
 {
 	/* FIXME: check codec capabilities of the phone */
 
@@ -75,7 +78,7 @@  uint8_t mncc_codec_for_mode(int lchan_type)
 		return mncc_int.def_codec[1];
 }
 
-static uint8_t determine_lchan_mode(struct gsm_mncc *setup)
+static enum gsm48_chan_mode determine_lchan_mode(struct gsm_mncc *setup)
 {
 	return mncc_codec_for_mode(setup->lchan_type);
 }
@@ -138,7 +141,8 @@  static int mncc_setup_ind(struct gsm_call *call, int msg_type,
 	memset(&mncc, 0, sizeof(struct gsm_mncc));
 	mncc.callref = call->callref;
 	mncc.lchan_mode = determine_lchan_mode(setup);
-	DEBUGP(DMNCC, "(call %x) Modify channel mode.\n", call->callref);
+	DEBUGP(DMNCC, "(call %x) Modify channel mode: %s\n", call->callref,
+	       osmo_gsm48_chan_mode2str(mncc.lchan_mode));
 	mncc_tx_to_cc(call->net, MNCC_LCHAN_MODIFY, &mncc);
 
 	/* send setup to remote */
@@ -207,13 +211,16 @@  static int mncc_setup_cnf(struct gsm_call *call, int msg_type,
 	DEBUGP(DMNCC, "(call %x) Bridging with remote.\n", call->callref);
 
 	/* in direct mode, we always have to bridge the channels */
-	if (ipacc_rtp_direct)
+	if (ipacc_rtp_direct) {
+		DEBUGP(DMNCC, "Bridging: direct RTP.\n");
 		return mncc_tx_to_cc(call->net, MNCC_BRIDGE, &bridge);
+	}
 
 	/* proxy mode */
 	if (!net->handover.active) {
 		/* in the no-handover case, we can bridge, i.e. use
 		 * the old RTP proxy code */
+		DEBUGP(DMNCC, "Bridging: no handover is active.\n");
 		return mncc_tx_to_cc(call->net, MNCC_BRIDGE, &bridge);
 	} else {
 		/* in case of handover, we need to re-write the RTP
diff --git a/openbsc/src/osmo-nitb/bsc_hack.c b/openbsc/src/osmo-nitb/bsc_hack.c
index dffe642..0b360dc 100644
--- a/openbsc/src/osmo-nitb/bsc_hack.c
+++ b/openbsc/src/osmo-nitb/bsc_hack.c
@@ -288,8 +288,10 @@  int main(int argc, char **argv)
 		rc = bsc_bootstrap_network(mncc_sock_from_cc, config_file);
 		if (rc >= 0)
 			mncc_sock_init(bsc_gsmnet, mncc_sock_path);
-	} else
+	} else {
+		DEBUGP(DMNCC, "Using internal MNCC handler.\n");
 		rc = bsc_bootstrap_network(int_mncc_recv, config_file);
+	}
 	if (rc < 0)
 		exit(1);
 #ifdef BUILD_SMPP