diff mbox

Refactor MNCC code

Message ID 1461579737-28325-1-git-send-email-msuraev@sysmocom.de
State New
Headers show

Commit Message

Max April 25, 2016, 10:22 a.m. UTC 
From: Max <msuraev@sysmocom.de>

* Explicitly use gsm_chan_t enum when checking for full rate channels.
* Consistently use enums instead of (u)int, char etc.
Note: because actual enum representation is up to compiler this might
change the size of gsm_mncc struct so the MNCC protocol version is
bumped.
---
 openbsc/include/openbsc/bsc_api.h     |  6 +++-
 openbsc/include/openbsc/gsm_04_08.h   |  2 +-
 openbsc/include/openbsc/mncc.h        |  8 ++++--
 openbsc/include/openbsc/mncc_int.h    |  5 +++-
 openbsc/src/libbsc/bsc_api.c          | 53 ++++++++++++++++++-----------------
 openbsc/src/libbsc/gsm_04_08_utils.c  |  2 +-
 openbsc/src/libmsc/gsm_04_08.c        |  5 ++--
 openbsc/src/libmsc/mncc_builtin.c     |  7 +++--
 openbsc/src/osmo-bsc/osmo_bsc_bssap.c | 10 ++++---
 9 files changed, 56 insertions(+), 42 deletions(-)

Comments

Harald Welte April 25, 2016, 1:03 p.m. UTC | #1 
Hi Max,

On Mon, Apr 25, 2016 at 12:22:17PM +0200, msuraev@sysmocom.de wrote:
> * Explicitly use gsm_chan_t enum when checking for full rate channels.
> * Consistently use enums instead of (u)int, char etc.

Is this supposed to be a pure cleanup patch (not changing behavior) or a
bug fix (changing behavior)?  This is currently not clear to me

I'm not particularly happy that in many cases when there is a bug to
fix, you first seem to restructure/cleanup the code and then address the
bug.  This adds the risk of introducing behavioral changes in the first
step (particularly with lack of good test coverage), and it also means
we're focussing less on the relevant topic.

> Note: because actual enum representation is up to compiler this might
> change the size of gsm_mncc struct so the MNCC protocol version is
> bumped.

I think this kind of change just creates more effort for everyone and I
don't really see much benefit of it.  You would have to merge related
changes not only to the NITB but to all its users (lcr, mncc_python,
osmo-sip-connector, ...) at the same time.  Have you changed all the
respective users and re-tested them?  Do you think it is a good use of
our time to do so for a merely stylistic improvement?

In general I think it is a bad idea to have enum's in structures that
describe an external interface / protocol betweeo two programs, exactly
for the reason you stated.  Alignment differences can be easily avoided
with __attribute__ ((packed)), but enums have the inherent problem of
'-fshort-enums', which might be default on some platform.  and then you
add a new enum value, and your structure size changes again.

Regards,
	Harald
Max April 25, 2016, 1:29 p.m. UTC | #2 
Yes, it's just rewrite of a cleanup from before which was not accepted
because it had booleans in function signature so it's safe to just drop.
The actual fix for a bug is in separate patch which does not depend on
this one.

Before fixing a bug I've got to first understand what existing code does
which is not always immediately clear, especially if comments are
missing. While going over a code I sometimes change/simplify functions
more as a "note to self" to make sure I easier understand particular
part of the code when getting back to it. If I feel that such changes
might be generally useful than I send it as a patch. Of course such
feeling is not always 100% match with what others feel so not all the
patches go through.

On 04/25/2016 03:03 PM, Harald Welte wrote:
> Hi Max,
>
> On Mon, Apr 25, 2016 at 12:22:17PM +0200, msuraev@sysmocom.de wrote:
>> * Explicitly use gsm_chan_t enum when checking for full rate channels.
>> * Consistently use enums instead of (u)int, char etc.
> Is this supposed to be a pure cleanup patch (not changing behavior) or a
> bug fix (changing behavior)?  This is currently not clear to me
>
> I'm not particularly happy that in many cases when there is a bug to
> fix, you first seem to restructure/cleanup the code and then address the
> bug.  This adds the risk of introducing behavioral changes in the first
> step (particularly with lack of good test coverage), and it also means
> we're focussing less on the relevant topic.
>
>> Note: because actual enum representation is up to compiler this might
>> change the size of gsm_mncc struct so the MNCC protocol version is
>> bumped.
> I think this kind of change just creates more effort for everyone and I
> don't really see much benefit of it.  You would have to merge related
> changes not only to the NITB but to all its users (lcr, mncc_python,
> osmo-sip-connector, ...) at the same time.  Have you changed all the
> respective users and re-tested them?  Do you think it is a good use of
> our time to do so for a merely stylistic improvement?
>
> In general I think it is a bad idea to have enum's in structures that
> describe an external interface / protocol betweeo two programs, exactly
> for the reason you stated.  Alignment differences can be easily avoided
> with __attribute__ ((packed)), but enums have the inherent problem of
> '-fshort-enums', which might be default on some platform.  and then you
> add a new enum value, and your structure size changes again.
>
> Regards,
> 	Harald
diff mbox

Patch

diff --git a/openbsc/include/openbsc/bsc_api.h b/openbsc/include/openbsc/bsc_api.h
index a3d12f2..b2fc1ed 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 <osmocom/gsm/gsm_utils.h>
+
 #include "gsm_data.h"
 
 #define BSC_API_CONN_POL_ACCEPT	0
@@ -45,7 +47,9 @@  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,
+		       enum gsm_chan_t chan);
 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..70866fe 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 {
@@ -166,7 +168,7 @@  struct gsm_data_frame {
 	unsigned char	data[0];
 };
 
-#define MNCC_SOCK_VERSION	5
+#define MNCC_SOCK_VERSION	6
 struct gsm_mncc_hello {
 	uint32_t	msg_type;
 	uint32_t	version;
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 b8b5967..2608504 100644
--- a/openbsc/src/libbsc/bsc_api.c
+++ b/openbsc/src/libbsc/bsc_api.c
@@ -34,8 +34,9 @@ 
 #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>
+#include <osmocom/gsm/gsm_utils.h>
 
 #define GSM0808_T10_VALUE    6, 0
 
@@ -155,7 +156,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, enum gsm_chan_t chan)
 {
 	struct bsc_api *api;
 	api = conn->bts->network->bsc_api;
@@ -163,12 +164,13 @@  static void handle_mr_config(struct gsm_subscriber_connection *conn,
 	struct gsm48_multi_rate_conf *mr_conf;
 
 	if (api->mr_config)
-		return api->mr_config(conn, lchan, full_rate);
+		return api->mr_config(conn, lchan,
+				      (GSM_LCHAN_TCH_H == chan) ? 0 : 1);
 
-	if (full_rate)
-		mr = &lchan->ts->trx->bts->mr_full;
-	else
+	if (GSM_LCHAN_TCH_H == chan)
 		mr = &lchan->ts->trx->bts->mr_half;
+	else
+		mr = &lchan->ts->trx->bts->mr_full;
 
 	mr_conf = (struct gsm48_multi_rate_conf *) mr->gsm48_ie;
 	mr_conf->ver = 1;
@@ -197,12 +199,11 @@  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,
+				 enum gsm_chan_t chan_type)
 {
 	struct gsm_lchan *new_lchan;
-	int chan_type;
-
-	chan_type = full_rate ? GSM_LCHAN_TCH_F : GSM_LCHAN_TCH_H;
 
 	new_lchan = lchan_alloc(conn->bts, chan_type, 0);
 
@@ -223,7 +224,7 @@  static int handle_new_assignment(struct gsm_subscriber_connection *conn, int cha
 
 	/* handle AMR correctly */
 	if (chan_mode == GSM48_CMODE_SPEECH_AMR)
-		handle_mr_config(conn, new_lchan, full_rate);
+		handle_mr_config(conn, new_lchan, chan_type);
 
 	if (rsl_chan_activate_lchan(new_lchan, 0x1, 0) < 0) {
 		LOGP(DHO, LOGL_ERROR, "could not activate channel\n");
@@ -337,7 +338,9 @@  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,
+				 enum gsm_chan_t chan)
 {
 	switch (chan_mode) {
 	case GSM48_CMODE_SIGN:
@@ -348,16 +351,14 @@  static int chan_compat_with_mode(struct gsm_lchan *lchan, int chan_mode, int ful
 	case GSM48_CMODE_DATA_3k6:
 	case GSM48_CMODE_DATA_6k0:
 		/* these services can all run on TCH/H, but we may have
-		 * an explicit override by the 'full_rate' argument */
+		 * an explicit override by the explicit chan argument */
 		switch (lchan->type) {
 		case GSM_LCHAN_TCH_F:
 			return 1;
 		case GSM_LCHAN_TCH_H:
-			if (full_rate)
-				return 0;
-			else
+			if (GSM_LCHAN_TCH_H == chan)
 				return 1;
-			break;
+			return 0;
 		default:
 			return 0;
 		}
@@ -368,9 +369,7 @@  static int chan_compat_with_mode(struct gsm_lchan *lchan, int chan_mode, int ful
 		/* these services all explicitly require a TCH/F */
 		if (lchan->type == GSM_LCHAN_TCH_F)
 			return 1;
-		else
-			return 0;
-		break;
+		return 0;
 	}
 
 	return 0;
@@ -382,19 +381,21 @@  static int chan_compat_with_mode(struct gsm_lchan *lchan, int chan_mode, int ful
  * this is for audio handling only. In case the current channel does not allow
  * the selected mode a new one will be allocated.
  *
+ * If chan is not GSM_LCHAN_TCH_H than full rate is assumed.
+ *
  * 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, enum gsm_chan_t chan)
 {
-	struct bsc_api *api;
-	api = conn->bts->network->bsc_api;
+	struct bsc_api *api = conn->bts->network->bsc_api;
 
-	if (!chan_compat_with_mode(conn->lchan, chan_mode, full_rate)) {
-		if (handle_new_assignment(conn, chan_mode, full_rate) != 0)
+	if (!chan_compat_with_mode(conn->lchan, chan_mode, chan)) {
+		if (handle_new_assignment(conn, chan_mode, chan) != 0)
 			goto error;
 	} else {
 		if (chan_mode == GSM48_CMODE_SPEECH_AMR)
-			handle_mr_config(conn, conn->lchan, full_rate);
+			handle_mr_config(conn, conn->lchan, chan);
 
 		LOGP(DMSC, LOGL_NOTICE,
 		     "Sending ChanModify for speech: %s on channel %s\n",
diff --git a/openbsc/src/libbsc/gsm_04_08_utils.c b/openbsc/src/libbsc/gsm_04_08_utils.c
index 635665a..327cff1 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_lchan_modify(struct gsm_lchan *lchan, uint8_t mode)
+int gsm48_lchan_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));
diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c
index 7b78d48..4de3477 100644
--- a/openbsc/src/libmsc/gsm_04_08.c
+++ b/openbsc/src/libmsc/gsm_04_08.c
@@ -2939,7 +2939,7 @@  static int _gsm48_lchan_modify(struct gsm_trans *trans, void *arg)
 		return 0;
 
 	return gsm0808_assign_req(trans->conn, mode->lchan_mode,
-		trans->conn->lchan->type != GSM_LCHAN_TCH_H);
+				  trans->conn->lchan->type);
 }
 
 static void mncc_recv_rtp(struct gsm_network *net, uint32_t callref,
@@ -3048,8 +3048,7 @@  static int tch_rtp_create(struct gsm_network *net, uint32_t callref)
 		LOGP(DMNCC, LOGL_DEBUG, "RTP create: codec=%s, chan_type=%s\n",
 		     get_value_string(gsm48_chan_mode_names, m),
 		     get_value_string(gsm_chan_t_names, lchan->type));
-		return gsm0808_assign_req(trans->conn, m,
-				lchan->type != GSM_LCHAN_TCH_H);
+		return gsm0808_assign_req(trans->conn, m, lchan->type);
 	}
 
 	mncc_recv_rtp_sock(trans->net, trans, MNCC_RTP_CREATE);
diff --git a/openbsc/src/libmsc/mncc_builtin.c b/openbsc/src/libmsc/mncc_builtin.c
index ee98d2d..b879756 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);
 }
diff --git a/openbsc/src/osmo-bsc/osmo_bsc_bssap.c b/openbsc/src/osmo-bsc/osmo_bsc_bssap.c
index a60940d..01105a8 100644
--- a/openbsc/src/osmo-bsc/osmo_bsc_bssap.c
+++ b/openbsc/src/osmo-bsc/osmo_bsc_bssap.c
@@ -28,6 +28,7 @@ 
 
 #include <osmocom/gsm/protocol/gsm_08_08.h>
 #include <osmocom/gsm/gsm0808.h>
+#include <osmocom/gsm/gsm_utils.h>
 
 /*
  * helpers for the assignment command
@@ -292,7 +293,8 @@  static int bssmap_handle_assignm_req(struct osmo_bsc_sccp_con *conn,
 	uint8_t timeslot;
 	uint8_t multiplex;
 	enum gsm48_chan_mode chan_mode = GSM48_CMODE_SIGN;
-	int i, supported, port, full_rate = -1;
+	enum gsm_chan_t chan = GSM_LCHAN_TCH_H;
+	int i, supported, port;
 
 	if (!conn->conn) {
 		LOGP(DMSC, LOGL_ERROR, "No lchan/msc_data in cipher mode command.\n");
@@ -344,7 +346,6 @@  static int bssmap_handle_assignm_req(struct osmo_bsc_sccp_con *conn,
 	 * inner loop. The outer loop will exit due chan_mode having
 	 * the correct value.
 	 */
-	full_rate = 0;
 	msc = conn->msc;
 	for (supported = 0;
 		chan_mode == GSM48_CMODE_SIGN && supported < msc->audio_length;
@@ -354,7 +355,8 @@  static int bssmap_handle_assignm_req(struct osmo_bsc_sccp_con *conn,
 		for (i = 2; i < TLVP_LEN(&tp, GSM0808_IE_CHANNEL_TYPE); ++i) {
 			if ((data[i] & 0x7f) == perm_val) {
 				chan_mode = gsm88_to_chan_mode(perm_val);
-				full_rate = (data[i] & 0x4) == 0;
+				if (0 == (data[i] & 0x4))
+					chan = GSM_LCHAN_TCH_F;
 				break;
 			} else if ((data[i] & 0x80) == 0x00) {
 				break;
@@ -371,7 +373,7 @@  static int bssmap_handle_assignm_req(struct osmo_bsc_sccp_con *conn,
 	port = mgcp_timeslot_to_endpoint(multiplex, timeslot);
 	conn->rtp_port = rtp_calculate_port(port, msc->rtp_base);
 
-	return gsm0808_assign_req(conn->conn, chan_mode, full_rate);
+	return gsm0808_assign_req(conn->conn, chan_mode, chan);
 
 reject:
 	resp = gsm0808_create_assignment_failure(GSM0808_CAUSE_NO_RADIO_RESOURCE_AVAILABLE, NULL);