diff mbox

[1/2] Refactor mncc code

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

Commit Message

Max March 24, 2016, 11:05 a.m. UTC 
From: Max <msuraev@sysmocom.de>

Use bool for boolean types.
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.
---
 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         | 17 +++++++++--------
 openbsc/src/libbsc/gsm_04_08_utils.c | 12 +++---------
 openbsc/src/libmsc/mncc_builtin.c    |  7 +++++--
 7 files changed, 29 insertions(+), 24 deletions(-)

Comments

Holger Freyther April 5, 2016, 8:47 p.m. UTC | #1 
> On 24 Mar 2016, at 12:05, msuraev@sysmocom.de wrote:
> 
> From: Max <msuraev@sysmocom.de>
> 
> Use bool for boolean types.
> 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.

	=> bump protocol version then


> -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);


we had this out of band. The topic is "boolean" trap, so if you want to change, then also consider using enums with just two values to express the meaning even stronger (and allow for extension).

E.g. with "full rate" it might turn into a tri-state

	* You have to use a TCH/F
	* You have to use a TCH/H
	* It would be nice to use a TCH/F..

> 
> -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);
> }


hehe, split this into a separate patch. Yes, it is obvious it returns rc either way and let's assume we don't have a unsigned -> int kind of conversion here.
Max April 6, 2016, 1:54 p.m. UTC | #2 
On 04/05/2016 10:47 PM, Holger Freyther wrote:
>
> -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);
> }
>
> hehe, split this into a separate patch. Yes, it is obvious it returns rc either way and let's assume we don't have a unsigned -> int kind of conversion here.
>
>
Can we remove this wrapper altogether and just use the function directly?
Also, no need to assume anything - both gsm48_tx_chan_mode_modify() and
wrapper are returning int so there is no conversion.
Holger Freyther April 6, 2016, 7:07 p.m. UTC | #3 
> On 06 Apr 2016, at 15:54, Max <msuraev@sysmocom.de> wrote:
> 
> 


> Can we remove this wrapper altogether and just use the function directly?
> Also, no need to assume anything - both gsm48_tx_chan_mode_modify() and
> wrapper are returning int so there is no conversion.

we could, but which name to pick? Before 2009 there was a difference between as we called an ipaccess function in one but not the other.
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..9ddd647 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;
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/mncc_builtin.c b/openbsc/src/libmsc/mncc_builtin.c
index 77df6fb..0942304 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);
 }