[MERGED] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: mgcp: use osmo-mgw to switch rtp streams
..


mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Depends: osmo-iuh I3c1a0455c5f25cae41ee19229d6daf299e023062
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/iucs_ranap.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
15 files changed, 1,316 insertions(+), 325 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 88305db..d31883a 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -20,6 +20,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
oap_client.h \
openbscdefines.h \
a_reset.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 2cddd25..3f322b3 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -188,9 +188,17 @@
uint8_t n_sd_next[4];
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..ac3283c
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,56 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context. This context information stores all information
+ * to handle the direction of the RTP streams via MGCP. There is one instance
+ * of this context struct per subscriber connection.
+ * (see also struct gsm_subscriber_connection) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number. This 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Harald Welte]

Patch Set 22: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/4980
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
Gerrit-PatchSet: 22
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: dexter 
Gerrit-Reviewer: lynxis lazus 
Gerrit-HasComments: No

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#19).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Depends: osmo-iuh I3c1a0455c5f25cae41ee19229d6daf299e023062
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/iucs_ranap.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
15 files changed, 1,316 insertions(+), 325 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/19

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 1b0bff9..98fa125 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..ac3283c
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,56 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context. This context information stores all information
+ * to handle the direction of the RTP streams via MGCP. There is one instance
+ * of this context struct per subscriber connection.
+ * (see also struct gsm_subscriber_connection) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number. This number identifies the 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Neels Hofmeyr]

Patch Set 18:

(3 comments)

updates from 34c3...

https://gerrit.osmocom.org/#/c/4980/18/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 141: static void handle_error(struct mgcp_ctx *mgcp_ctx, enum 
msc_mgcp_cause_code cause)
on 34c3 I noticed that error logging doesn't convey the actual source of the 
error, so I added 
http://git.osmocom.org/osmo-msc/commit/?h=neels/34c3=f291d2f007f00f466b3c3299864b79997b13395d
 which logs file and line of the handle_error() caller


Line 929:   OSMO_ASSERT(conn);
on 34c3 we apparently saw the need to add 
http://git.osmocom.org/osmo-msc/commit/?h=34c3=d251b5379c1c9309f37ab159275ad2e9694a6c38

an assert would crash the MSC, maybe that 34c3 patch would be better? not sure 
why conn==NULL would be hit though.


Line 953: 
on 34c3 we apparently saw the need for 
http://git.osmocom.org/osmo-msc/commit/?h=34c3=02dd4425a01b0fab069acfe3a2940d9e5e7fc085


-- 
To view, visit https://gerrit.osmocom.org/4980
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
Gerrit-PatchSet: 18
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: dexter 
Gerrit-Reviewer: lynxis lazus 
Gerrit-HasComments: Yes

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#18).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Depends: osmo-iuh I3c1a0455c5f25cae41ee19229d6daf299e023062
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/iucs_ranap.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
15 files changed, 1,309 insertions(+), 325 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/18

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 1b0bff9..98fa125 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..ac3283c
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,56 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context. This context information stores all information
+ * to handle the direction of the RTP streams via MGCP. There is one instance
+ * of this context struct per subscriber connection.
+ * (see also struct gsm_subscriber_connection) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number. This number identifies the 

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#17).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Depends: osmo-iuh I3c1a0455c5f25cae41ee19229d6daf299e023062
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/iucs_ranap.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
15 files changed, 1,310 insertions(+), 325 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/17

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 1b0bff9..98fa125 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..ac3283c
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,56 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context. This context information stores all information
+ * to handle the direction of the RTP streams via MGCP. There is one instance
+ * of this context struct per subscriber connection.
+ * (see also struct gsm_subscriber_connection) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number. This number identifies the 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Neels Hofmeyr]

Patch Set 16: Code-Review+1

(14 comments)

I am still finding things (you know me) ... but I think it is becoming not 
worth it to fix them before merging. We've really had more than enough passes 
on it, from your comments this patch seems to work well enough, and my comments 
can all be addressed later. Even if they get dropped on the floor, maybe this 
will help improving future patches.

I would welcome for the future that you actually go on gerrit and read through 
your patch like a reviewer, and you should find the typos I keep marking, by 
yourself, that would be cool.

Here follow the comments, but if another reviewer agrees that none of them are 
worthwhile another cycle, I'd be ok with merging as-is.

I think the most important one is "Line 903".

https://gerrit.osmocom.org/#/c/4980/16/include/osmocom/msc/msc_mgcp.h
File include/osmocom/msc/msc_mgcp.h:

Line 34:/* RTP endpoint number. This number number identifies the 
endpoint
number number


https://gerrit.osmocom.org/#/c/4980/16/src/libmsc/a_iface.c
File src/libmsc/a_iface.c:

Line 403:   LOGP(DMSC, LOGL_ERROR,
would be nice to use LOGPCONN as above in line 392


https://gerrit.osmocom.org/#/c/4980/16/src/libmsc/iucs.c
File src/libmsc/iucs.c:

Line 42: extern struct msgb *ranap_new_msg_rab_assign_voice(uint8_t rab_id,
wondering why this is necessary, typically we would #include a header with this 
definition. extern is not necessary (I'm not really sure why extern exists as a 
keyword for functions)


https://gerrit.osmocom.org/#/c/4980/16/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 183:   osmo_fsm_inst_dispatch(fi, EV_TEARDOWN_ERROR, mgcp_ctx);
I still think these two lines would make a good common static function like 
mgcp_fsm_error() ... wait, you actually have a handle_error() function, isn't 
it applicable here?


Line 742:   LOGPFSML(fi, LOGL_DEBUG, "state machine halted\n");
(the FSM internal logging just logged a state transition to ST_HALT, this is 
duplicating that, right?)


Line 757:   osmo_fsm_inst_state_chg(fi, ST_HALT, MGCP_REL_TIMEOUT, 
MGCP_REL_TIMEOUT_TIMER_NR);
fsm_halt_cb is only invoked from receving events in the ST_HALT state. Why 
re-enter ST_HALT from ST_HALT?

>From another angle: above comment says "state machine halted", yet there is 
>another state change triggered?


Line 763:   [ST_CRCX_RAN] = {
Thanks for the improvements, they help to understand.

I still find that in the naming of the states, there is a mixup between the 
action, state and event. When an action like "sending X" happened, a state is 
like "waiting for Y", and event then is "received Y". Here the FSM states' 
naming tend to be like the action that came before entering a state or the 
event that triggered leaving the previous state. So when we're actually waiting 
for Y, the state is named "send X".

In this particular state, the FSM init code probably sent the CRCX for RAN, and 
now this state could be ST_WAIT_CRCX_RAN_RESP. The comment above suggests that 
entering the state implicitly sends the CRCX (theoretically possible with an 
on_enter cb, but there is none).

Other example: in handle_error(), we enter the ST_CALL state. 
That reads like: on any error, we have a call established.

So, what I mean is, when reading, I find that questions remain that can only be 
answered by reading more of the code, and it would be nice to clearly know who 
did the action (the state doesn't do actions, it's just a number) and what the 
state is waiting for by the name and comment alone.


Line 770:  sending the CRCX for CN side */
*


Line 797:* completed we will check the IP/Port of the RAN connection. 
If we
as completed


Line 798:* this data is valid we may continue with the MDCX phase for 
the RAN
If we this


Line 799:* side. If not we wait until the assinment completes on the A 
or on
assinment


Line 816:   /* The ran side MDCX phase is complete when the response is 
received
'ran' the verb or RAN the acronym


Line 817:* from the MGW. The is then active and we change to ST_CALL 
and wait
The is


Line 903:   mgcp_ctx->fsm = osmo_fsm_inst_alloc(_msc_mgcp, NULL, NULL, 
LOGL_DEBUG, name);
I still think it should be a child fsm of conn->conn_fsm


-- 
To view, visit https://gerrit.osmocom.org/4980
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
Gerrit-PatchSet: 16
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: dexter 
Gerrit-HasComments: Yes

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#14).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Depends: osmo-iuh I3c1a0455c5f25cae41ee19229d6daf299e023062
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/iucs_ranap.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
15 files changed, 1,317 insertions(+), 325 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/14

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..2fd44ef
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,56 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context. This context information stores all information
+ * to handle the direction of the RTP streams via MGCP. There is one instance
+ * of this context struct per subscriber connection.
+ * (see also struct gsm_subscriber_connection) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number. This number number identifies 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Neels Hofmeyr]

Patch Set 13:

(2 comments)

https://gerrit.osmocom.org/#/c/4980/13/src/libmsc/iucs.c
File src/libmsc/iucs.c:

Line 224:   LOGP(DIUCS, LOGL_DEBUG,
(here's another candidate for a common LOGPFOO() context macro ... probably 
also applies for the rest of this file; so I'd pass this patch without one and 
change over to a proper macro separately)


https://gerrit.osmocom.org/#/c/4980/13/src/libmsc/iucs_ranap.c
File src/libmsc/iucs_ranap.c:

Line 53:printf("transportLayerAddress present!\n");
-1: drop the printf, but I know you're working on that FIXME at the moment, so 
waiting for the next patch set.


-- 
To view, visit https://gerrit.osmocom.org/4980
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
Gerrit-PatchSet: 13
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-Reviewer: dexter 
Gerrit-HasComments: Yes

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#13).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/iucs_ranap.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
15 files changed, 1,289 insertions(+), 321 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/13

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..2fd44ef
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,56 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context. This context information stores all information
+ * to handle the direction of the RTP streams via MGCP. There is one instance
+ * of this context struct per subscriber connection.
+ * (see also struct gsm_subscriber_connection) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number. This number number identifies the endpoint
+* on the MGW on which the RAN and CN 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Patch Set 11:

(70 comments)

Actually we should not merge this patch until we did not solve the problems 
with 3G (rab assignment response not parsed).

For the other topic, please see my email.

https://gerrit.osmocom.org/#/c/4980/2//COMMIT_MSG
Commit Message:

Line 15: Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
> please use the tag as "Depends: foo" to match general tag syntax like below
Done


https://gerrit.osmocom.org/#/c/4980/2/include/osmocom/msc/msc_ifaces.h
File include/osmocom/msc/msc_ifaces.h:

Line 44
> -1: msc_ifaces.h/c didn't really turn out as clearly as I had intended... B
Done


https://gerrit.osmocom.org/#/c/4980/2/include/osmocom/msc/msc_mgcp.h
File include/osmocom/msc/msc_mgcp.h:

Line 33: 
> ?: I wonder whether rtp_endpoint and the conn_ids should rather be part of 
The structs are similar, however I would not include this into the mgcp_client 
API. It is not guaranteed that every client will use exactly one connection. 
Integrating this into the client would require some more elaborated approach. 
We would have to manage the connection IDs some how, which means we would have 
to track open connections, add CRCX and remove connection IDs to lists on DLCX. 
This would be rather complex. I think letting the user tracking the connection 
IDs is pretty ok.


https://gerrit.osmocom.org/#/c/4980/11/include/osmocom/msc/msc_mgcp.h
File include/osmocom/msc/msc_mgcp.h:

Line 26: /* MGCP state handler context (fsm etc..) */
> explain the scope? mgcp context for one call leg? one subscriber?
Done


Line 31:/* RTP endpoint number */
> (omit comments that just mirror the variable name; explain what the endpoin
Done


Line 35: * needed */
> use line width instead of wrapping short lines
Done


https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/a_iface.c
File src/libmsc/a_iface.c:

Line 412:   rtp_addr_in.sin_addr.s_addr = 
inet_addr(conn->rtp.local_addr_ran);
> needs some error checking. man inet_addr:
Done


https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/gsm_04_08.c
File src/libmsc/gsm_04_08.c:

Line 1333:  /* Initiate the teadown of the related connections on the MGW */
> "teadown"
Done


Line 1390: /* helper function for tch_bridge() to bridge the RTP Voice streams 
also */
> (I find "helper function for" is something you could write everywhere; I al
The "helper function for" serves basically the purpose to mark the function as 
a nested function. Unfortunately we do not have nested functions in C (see also 
pascal).

Actually we do not need to have this in a separate function anyway so I removed 
it.


Line 2691:  uint32_t addr = inet_addr(trans->conn->rtp.local_addr_cn);
> inet_addr, error checking
Done


https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/iucs.c
File src/libmsc/iucs.c:

Line 226:" rtp=%x:%u, use_x213_nsap=%d\n", conn_id, rab_id, rtp_ip,
> I would have said, use line width, but this is just moving code, right?
I have corrected the formatting of the whole function now. This is code that 
had been moved from msc_ifaces.c to here.

(splitting the log strings is a coding style violation anyway, so I 
concatenated the strings again and corrected the the formatting)


Line 235:" conn_id=%d rab_id=%d rtp=%x:%u\n",
> same
Done


https://gerrit.osmocom.org/#/c/4980/2/src/libmsc/msc_ifaces.c
File src/libmsc/msc_ifaces.c:

Line 194
> looks like callers should just call msc_mgcp_call_assignment() directly.
Done


Line 249
> looks like callers should just call msc_mgcp_call_release() directly.
Done


https://gerrit.osmocom.org/#/c/4980/2/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 47: #define MGCP_RAN_TIMEOUT 10/* in seconds */
> -1: prefer msc_mgcp_ instead of int_ because if you follow this scheme, we 
Done


Line 59:MGCP_ERR_MGW_FAIL,
> -1: typical naming would be
Done


Line 93:ST_CALL,
> -1: again you are using the same general struct name, just 'fsm_', like in 
Done


Line 126:* the CN side */
> fsm_
Done


Line 217: {
> -1: again, you do not need to log FSM states or events, the FSM code does t
Done


Line 222:   int rc;
> -1: best use "0x%x" so we always know it is logged in hex. Are you sure it 
osmo-mgw parses the numerical digit as hexadecimal number, so yes we should 
display it in hexadecimal here too.


Line 240:   if (snprintf(mgcp_msg.endpoint, MGCP_ENDPOINT_MAXLEN, 
MGCP_ENDPOINT_FORMAT, mgcp_ctx->rtp_endpoint) >=
> (wondering whether this log line adds any info to the log, since the FSM al
Done


Line 248:   /* Transmit MGCP message to MGW */
> lol, return at the end of a void function? (some more below)
Done


Line 319:   handle_error(mgcp_ctx, MGCP_ERR_UNEXP_TEARDOWN);
> nope
Done


Line 422:   goto error;
> nope
Done


Line 442:* of the side pointing towards the BSS should be already 
communicated
> (code dup: could have one handle_error(...) below and goto assignment_fail 
Done


Line 460:   struct 

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#12).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
14 files changed, 1,279 insertions(+), 321 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/12

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..2fd44ef
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,56 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context. This context information stores all information
+ * to handle the direction of the RTP streams via MGCP. There is one instance
+ * of this context struct per subscriber connection.
+ * (see also struct gsm_subscriber_connection) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number. This number number identifies the endpoint
+* on the MGW on which the RAN and CN connection is created. This
+ 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Neels Hofmeyr]

Patch Set 11: Code-Review-1

(31 comments)

In general, I wonder whether it is helping that I comment on the numerous 
cosmetic issues. Most of it could be caught by you reading your own patch, it 
is taking a lot of time of my day and it seems like I am reiterating similar 
obvious points over and over. It makes me feel like a nitpick and probably 
makes you feel unappreciated, also clutters the really important code review 
and probably makes me miss the real problems; IOW, I would enjoy to give more 
constructive feedback so that we can both feel good about the review. Either by 
me letting every cosmetic no-go pass uncommented or by you fixing them before 
submitting a patch; not sure which it should be, maybe a bit of both; maybe 
others have an opinion on that too.

I'd like to finally see this merged, but still can't +2 because:

- I don't understand the FSM -- it should be quite linear, but it reads like it 
has paths crossing and teardown is skipping/merging some steps that should be 
separate. If the FSM is correct and I'm not seeing it, then the comments and 
state/event naming could help clarifying; see inline comments.

- missing inet_addr() error handling / needs replacement.

https://gerrit.osmocom.org/#/c/4980/11/include/osmocom/msc/msc_mgcp.h
File include/osmocom/msc/msc_mgcp.h:

Line 26: /* MGCP state handler context (fsm etc..) */
explain the scope? mgcp context for one call leg? one subscriber?


Line 31:/* RTP endpoint number */
(omit comments that just mirror the variable name; explain what the endpoint is 
about or drop the comment?)


Line 35: * needed */
use line width instead of wrapping short lines


https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/a_iface.c
File src/libmsc/a_iface.c:

Line 412:   rtp_addr_in.sin_addr.s_addr = 
inet_addr(conn->rtp.local_addr_ran);
needs some error checking. man inet_addr:

   The  inet_addr()  function  converts  the  Internet  host  address cp 
from IPv4 numbers-and-dots notation into binary data in network byte order.  If 
the input is invalid, INADDR_NONE (usually -1) is
   returned.  Use of this function is problematic because -1 is a valid 
address (255.255.255.255).  Avoid its use in favor of inet_aton(), 
inet_pton(3), or getaddrinfo(3), which provide a cleaner way to
   indicate error return.


https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/gsm_04_08.c
File src/libmsc/gsm_04_08.c:

Line 1333:  /* Initiate the teadown of the related connections on the MGW */
"teadown"


Line 1390: /* helper function for tch_bridge() to bridge the RTP Voice streams 
also */
(I find "helper function for" is something you could write everywhere; I also 
used to write this phrase a lot some years ago, but better just say what it 
does)


Line 2691:  uint32_t addr = inet_addr(trans->conn->rtp.local_addr_cn);
inet_addr, error checking


https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/iucs.c
File src/libmsc/iucs.c:

Line 226:" rtp=%x:%u, use_x213_nsap=%d\n", conn_id, rab_id, rtp_ip,
I would have said, use line width, but this is just moving code, right?


Line 235:" conn_id=%d rab_id=%d rtp=%x:%u\n",
same


https://gerrit.osmocom.org/#/c/4980/11/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 56: /* Some internal cause codes to indicate fault
use line width


Line 70: /* Human readable respresentation of the faul codes,
line width, also multiple more times below


Line 102:* may now forward IP/Port of the remote call leg to the MGW*/
(space before '*/')


Line 167:   if (fi->T == MGCP_MGW_TIMEOUT_TIMER_NR) {
(a lot of "Note:" below)


Line 180:   osmo_fsm_inst_dispatch(fi, EV_TEARDOWN, mgcp_ctx);
could make sense to place teardown in a ST_HALT -> on_enter function? Otherwise 
could make sense to combine the state_chg + event dispatch in a common function


Line 231:"CRCX/RAN: creating connection for the RAN side on " 
"MGW endpoint:0x%x...\n", mgcp_ctx->rtp_endpoint);
join "..." "..."


Line 710:   osmo_fsm_inst_state_chg(fi, ST_HALT, MGCP_MGW_TIMEOUT, 
MGCP_MGW_TIMEOUT_TIMER_NR);
above, a TEARDOWN event follows after the HALT state_chg. What triggers the 
teardown here?


Line 747:   if (mgcp_ctx->free_ctx) {
the free_ctx flag essentially creates two kinds of HALT states. One where we're 
still waiting for something (but what?) and one where we're going to free on 
any (!) incoming event? I would prefer if the waiting-for-x and halting + 
freeing states were explicit and distinct states, instead of using the same FSM 
state split up by additional ctx flag. Also would welcome if it was obvious 
which kinds of events are expected to arrive here, and that the event is 
distinct. For example, it looks now like the teardown event could happen in any 
context, and once it means "go to HALT state" (does it?) and once it means 
"let's discard the FSM right now"... would be better to have explicit and 
distinct events.


Line 749:

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#11).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
14 files changed, 1,228 insertions(+), 321 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/11

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..0ccd838
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,52 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* Set to true, when the context information is no longer
+* needed */
+   bool free_ctx;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_LENGTH];
+   char conn_id_cn[MGCP_CONN_ID_LENGTH];
+
+   /* Copy of the pointer and the data with 

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#10).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
14 files changed, 1,227 insertions(+), 321 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/10

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..0ccd838
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,52 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+#include 
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* Set to true, when the context information is no longer
+* needed */
+   bool free_ctx;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_LENGTH];
+   char conn_id_cn[MGCP_CONN_ID_LENGTH];
+
+   /* Copy of the pointer and the data with 

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#9).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
M tests/msc_vlr/Makefile.am
M tests/msc_vlr/msc_vlr_tests.c
14 files changed, 1,121 insertions(+), 321 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/9

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..a48a316 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -1,7 +1,10 @@
 #pragma once
 
+#include 
+
 int gsm0408_rcvmsg_iucs(struct gsm_network *network, struct msgb *msg,
uint16_t *lac);
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..da06dae
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,50 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+#include 
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* Set to true, when the context information is no longer
+* needed */
+   bool free_ctx;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_LENGTH];
+   char conn_id_cn[MGCP_CONN_ID_LENGTH];
+
+   /* Copy of the pointer and the data with context 

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#8).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
12 files changed, 1,087 insertions(+), 309 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/8

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..2f3905e 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -5,3 +5,5 @@
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+struct gsm_trans;
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..907e303
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,47 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* Set to true, when the context information is no longer
+* needed */
+   bool free_ctx;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_LENGTH];
+   char conn_id_cn[MGCP_CONN_ID_LENGTH];
+
+   /* Copy of the pointer and the data with context information
+* needed to process the AoIP and MGCP requests (system data) */
+   struct mgcp_client *mgcp;
+   struct gsm_trans *trans;
+};
+
+int msc_mgcp_call_assignment(struct gsm_trans 

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#6).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
12 files changed, 1,087 insertions(+), 309 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/6

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 696cef1..9106421 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -196,9 +196,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..9c6c858 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -5,3 +5,5 @@
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..907e303
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,47 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* Set to true, when the context information is no longer
+* needed */
+   bool free_ctx;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_LENGTH];
+   char conn_id_cn[MGCP_CONN_ID_LENGTH];
+
+   /* Copy of the pointer and the data with context information
+* needed to process the AoIP and MGCP requests (system data) */
+   struct mgcp_client *mgcp;
+   struct gsm_trans *trans;
+};
+
+int msc_mgcp_call_assignment(struct gsm_trans *trans);
+int 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Harald Welte]

Patch Set 5:

(1 comment)

https://gerrit.osmocom.org/#/c/4980/5/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 731:.in_event_mask = S(EV_TEARDOWN) | 
S(EV_DLCX_ALL_RESP),
the halt state when entered immediately terminates.  Yet we are accepting 
inbound events like DLCX_ALL_RESP only in that state.  How can that be?


-- 
To view, visit https://gerrit.osmocom.org/4980
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
Gerrit-PatchSet: 5
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#4).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
12 files changed, 1,024 insertions(+), 308 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/4

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index e0fbcec..bdbb448 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -195,9 +195,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..9c6c858 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -5,3 +5,5 @@
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..138538b
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,43 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_LENGTH];
+   char conn_id_cn[MGCP_CONN_ID_LENGTH];
+
+   /* Copy of the pointer and the data with context information
+* needed to process the AoIP and MGCP requests (system data) */
+   struct mgcp_client *mgcp;
+   struct gsm_trans *trans;
+};
+
+int msc_mgcp_call_assignment(struct gsm_trans *trans);
+int msc_mgcp_call_complete(struct gsm_trans *trans, uint16_t port, char *addr);
+int msc_mgcp_call_release(struct 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Neels Hofmeyr]

Patch Set 3: Code-Review-1

(8 comments)

excellent improvements. I still found two problems, a couple instances where I 
can accept if you oppose my previous comments but would like a comment on it if 
so. And few minor cosmetics.

https://gerrit.osmocom.org/#/c/4980/3/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 65: static const struct value_string msc_mgcp_cause_codes_str[] = {
(prefix 'msc_mgcp_': nice. Why not the common value_string[] suffix '_names'?)


Line 283:"CRCX/CN creating connection for the CN side on MGW 
endpoint:%0x%x...\n", mgcp_ctx->rtp_endpoint);
-1: oops: %0x%x


Line 389:   OSMO_ASSERT(false);
well, or we added a new RAN type and forgot to implement it here. I usually do 
error log like "Unknown RAN type: %d" and returning an error.


Line 445:conn->rtp.remote_port_cn);
(i.e. you don't want to combine these two logs?)


Line 529:conn->rtp.remote_port_ran);
(i.e. you don't want to combine these two logs?)


Line 803:   mgcp_ctx->fsm = osmo_fsm_inst_alloc(_msc_mgcp, NULL, NULL, 
LOGL_DEBUG, name);
i.e. you don't want the FSM to be a child of the conn_fsm?


Line 806:   LOGPFSML(mgcp_ctx->fsm, LOGL_DEBUG, "MGW handler fsm 
created\n");
(this is also a duplicate of FSM internal logging, right?)


Line 858:   LOGP(DMGCP, LOGL_ERROR, "invalid call state, call 
completion failed\n");
-1: context


-- 
To view, visit https://gerrit.osmocom.org/4980
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4980

to look at the new patch set (#3).

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends: osmo-mgw Iab6a6038e7610c62f34e642cd49c93d11151252c
Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/iucs.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/iucs.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
12 files changed, 1,031 insertions(+), 308 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/3

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index e0fbcec..bdbb448 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -195,9 +195,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/iucs.h b/include/osmocom/msc/iucs.h
index b7d6064..9c6c858 100644
--- a/include/osmocom/msc/iucs.h
+++ b/include/osmocom/msc/iucs.h
@@ -5,3 +5,5 @@
 
 struct gsm_subscriber_connection *subscr_conn_lookup_iu(struct gsm_network 
*network,
struct 
ranap_ue_conn_ctx *ue);
+
+int iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..ca25e9d 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -37,7 +37,3 @@
 enum gsm48_reject_value value);
 
 int msc_tx_common_id(struct gsm_subscriber_connection *conn);
-int msc_call_assignment(struct gsm_trans *trans);
-int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
-void msc_call_release(struct gsm_trans *trans);
-int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..7fb5411
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,43 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_MAXLEN];
+   char conn_id_cn[MGCP_CONN_ID_MAXLEN];
+
+   /* Copy of the pointer and the data with context information
+* needed to process the AoIP and MGCP requests (system data) */
+   struct mgcp_client *mgcp;
+   struct gsm_trans *trans;
+};
+
+int msc_mgcp_call_assignment(struct gsm_trans *trans);
+int msc_mgcp_call_complete(struct gsm_trans *trans, uint16_t port, char *addr);
+int msc_mgcp_call_release(struct 

osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[Neels Hofmeyr]

Patch Set 2: Code-Review-1

(31 comments)

https://gerrit.osmocom.org/#/c/4980/2//COMMIT_MSG
Commit Message:

Line 15: Depends osmo-mgw: Iab6a6038e7610c62f34e642cd49c93d11151252c
please use the tag as "Depends: foo" to match general tag syntax like below ... 
I usually use

  Depends: Iab6a6038e7610c62f34e642cd49c93d11151252c (osmo-mgw)

and don't separate from the other tags, i.e. drop the blank line


https://gerrit.osmocom.org/#/c/4980/2/include/osmocom/msc/msc_ifaces.h
File include/osmocom/msc/msc_ifaces.h:

Line 44: int msc_iu_rab_act_cs(struct gsm_trans *trans);
-1: msc_ifaces.h/c didn't really turn out as clearly as I had intended... But 
looking at this change I find that this function should be in iucs.h/c. Let's 
not add msc_ to the name and just add iu_rab_act_cs to iucs.h so that you can 
call it from the FSM code. Another patch can move the implementation from 
msc_ifaces.c to iucs.c (or this patch if you like)


https://gerrit.osmocom.org/#/c/4980/2/include/osmocom/msc/msc_mgcp.h
File include/osmocom/msc/msc_mgcp.h:

Line 33:char conn_id_cn[MGCP_CONN_ID_MAXLEN];
?: I wonder whether rtp_endpoint and the conn_ids should rather be part of 
struct mgcp_client? How does osmo-bsc solve it, does it have the same structure 
that can be unified between osmo-bsc and osmo-msc, i.e. in the mgcp_client API?


https://gerrit.osmocom.org/#/c/4980/2/src/libmsc/msc_ifaces.c
File src/libmsc/msc_ifaces.c:

Line 194:   return msc_mgcp_call_assignment(trans);
looks like callers should just call msc_mgcp_call_assignment() directly.


Line 249:   msc_mgcp_call_release(trans);
looks like callers should just call msc_mgcp_call_release() directly.


https://gerrit.osmocom.org/#/c/4980/2/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 47: enum int_cause_code {
-1: prefer msc_mgcp_ instead of int_ because if you follow this scheme, we will 
have int_ prefixes everywhere and it is harder to grep the code


Line 59: static const struct value_string int_cause_codes_str[] = {
-1: typical naming would be

  foo_names

like you use below, so
   
  msc_mgcp_cause_code_names[]


Line 93: enum fsm_evt {
-1: again you are using the same general struct name, just 'fsm_', like in 
osmo-bsc. I know it's a static context, but please give each FSM its own unique 
naming, like my patches that we merged into your osmo-bsc FSM patches.


Line 126: static const struct value_string fsm_evt_names[] = {
fsm_


Line 217:get_value_string(fsm_bsc_mgcp_state_names, fi->state), 
get_value_string(fsm_evt_names, event));
-1: again, you do not need to log FSM states or events, the FSM code does that. 
I've mailed you so before, let's not repeat this.


Line 222:"CRCX/RAN: creating connection for the RAN side on " 
"MGW endpoint:%x...\n", mgcp_ctx->rtp_endpoint);
-1: best use "0x%x" so we always know it is logged in hex. Are you sure it 
should be in hex though? In the VTY rtp endpoint ranges it isn't. same multiple 
times below.


Line 240:   LOGPFSML(fi, LOGL_DEBUG, "CRCX/RAN: transmitting MGCP message 
to MGW...\n");
(wondering whether this log line adds any info to the log, since the FSM 
already logged ST_CRCX_RAN. same multiple times below)


Line 248:   return;
lol, return at the end of a void function? (some more below)


Line 319:get_value_string(fsm_bsc_mgcp_state_names, fi->state), 
get_value_string(fsm_evt_names, event));
nope


Line 422:get_value_string(fsm_bsc_mgcp_state_names, fi->state), 
get_value_string(fsm_evt_names, event));
nope


Line 442:   handle_error(mgcp_ctx, MGCP_ERR_ASSGMNT_FAIL);
(code dup: could have one handle_error(...) below and goto assignment_fail from 
each failure)


Line 460:   /* Note: When we reach this point than the situation is 
basically that
(then)


Line 465:   osmo_fsm_inst_state_chg(fi, ST_MDCX_CN, 0, 0);
?: no timeout?


Line 492:get_value_string(fsm_bsc_mgcp_state_names, fi->state), 
get_value_string(fsm_evt_names, event));
...


Line 503:"MDCX/CN: completing connection for the CN side on " 
"MGW endpoint:%x...\n", mgcp_ctx->rtp_endpoint);
(line break or drop the " " in the middle)


Line 507:conn->rtp.remote_port_cn);
(rather combine with preceding LOGPFSML for less log lines and a bit less 
logging overhead)


Line 552:   OSMO_ASSERT(conn);
you're copying these lines and local variables around everywhere, even in 
functions where you don't use them at all, like this one, which is only using 
mgcp_ctx?


Line 609:conn->rtp.remote_port_ran);
(combine logs)


Line 691:   LOGPFSML(fi, LOGL_ERROR, "call active, waiting for 
teardown...\n");
really an error?


Line 803:.in_event_mask = (1 << EV_INIT),
(we often use an S() macro, e.g. see top of vlr_lu_fsm.c and struct 
sub_pres_vlr_states definition)


Line 805:.name = "ST_CRCX_RAN",
(I like to use 

[PATCH] osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

[dexter]

Review at  https://gerrit.osmocom.org/4980

mgcp: use osmo-mgw to switch rtp streams

in the current implementation we still use osmo-bsc_mgcp, which
has many problems and is also obsoleted by osmo-mgw.

integrate osmo-mgw and re-implement the current switching using
an osmo fsm.

Depends osmo-mgw: Iab6a6038e7610c62f34e642cd49c93d11151252c

Closes: OS#2605
Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
---
M include/osmocom/msc/Makefile.am
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/msc_ifaces.h
A include/osmocom/msc/msc_mgcp.h
M src/libmsc/Makefile.am
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/gsm_04_08.c
M src/libmsc/msc_ifaces.c
A src/libmsc/msc_mgcp.c
10 files changed, 1,122 insertions(+), 219 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/80/4980/1

diff --git a/include/osmocom/msc/Makefile.am b/include/osmocom/msc/Makefile.am
index 1419e8e..052d830 100644
--- a/include/osmocom/msc/Makefile.am
+++ b/include/osmocom/msc/Makefile.am
@@ -39,6 +39,7 @@
mncc.h \
mncc_int.h \
msc_ifaces.h \
+   msc_mgcp.h \
network_listen.h \
oap_client.h \
openbscdefines.h \
diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 4d493cb..5eecc15 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -195,9 +195,17 @@
struct gsm_encr encr;
 
struct {
+   struct mgcp_ctx *mgcp_ctx;
unsigned int mgcp_rtp_endpoint;
-   uint16_t port_subscr;
-   uint16_t port_cn;
+
+   uint16_t local_port_ran;
+   char local_addr_ran[INET_ADDRSTRLEN];
+   uint16_t remote_port_ran;
+   char remote_addr_ran[INET_ADDRSTRLEN];
+   uint16_t local_port_cn;
+   char local_addr_cn[INET_ADDRSTRLEN];
+   uint16_t remote_port_cn;
+   char remote_addr_cn[INET_ADDRSTRLEN];
} rtp;
 
/* which Iu-CS connection, if any. */
diff --git a/include/osmocom/msc/msc_ifaces.h b/include/osmocom/msc/msc_ifaces.h
index 0592c07..5e560b3 100644
--- a/include/osmocom/msc/msc_ifaces.h
+++ b/include/osmocom/msc/msc_ifaces.h
@@ -41,3 +41,4 @@
 int msc_call_bridge(struct gsm_trans *trans1, struct gsm_trans *trans2);
 void msc_call_release(struct gsm_trans *trans);
 int msc_call_connect(struct gsm_trans *trans, uint16_t port, uint32_t ip);
+int msc_iu_rab_act_cs(struct gsm_trans *trans);
diff --git a/include/osmocom/msc/msc_mgcp.h b/include/osmocom/msc/msc_mgcp.h
new file mode 100644
index 000..7fb5411
--- /dev/null
+++ b/include/osmocom/msc/msc_mgcp.h
@@ -0,0 +1,43 @@
+/* (C) 2017 by sysmocom - s.f.m.c. GmbH 
+ * All Rights Reserved
+ *
+ * Author: Philipp Maier
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see .
+ *
+ */
+
+#pragma once
+
+/* MGCP state handler context (fsm etc..) */
+struct mgcp_ctx {
+   /* FSM instance, which handles the connection switching procedure */
+   struct osmo_fsm_inst *fsm;
+
+   /* RTP endpoint number */
+   uint16_t rtp_endpoint;
+
+   /* RTP connection identifiers */
+   char conn_id_ran[MGCP_CONN_ID_MAXLEN];
+   char conn_id_cn[MGCP_CONN_ID_MAXLEN];
+
+   /* Copy of the pointer and the data with context information
+* needed to process the AoIP and MGCP requests (system data) */
+   struct mgcp_client *mgcp;
+   struct gsm_trans *trans;
+};
+
+int msc_mgcp_call_assignment(struct gsm_trans *trans);
+int msc_mgcp_call_complete(struct gsm_trans *trans, uint16_t port, char *addr);
+int msc_mgcp_call_release(struct gsm_trans *trans);
diff --git a/src/libmsc/Makefile.am b/src/libmsc/Makefile.am
index fee9f44..e872d03 100644
--- a/src/libmsc/Makefile.am
+++ b/src/libmsc/Makefile.am
@@ -40,6 +40,7 @@
mncc_builtin.c \
mncc_sock.c \
msc_ifaces.c \
+   msc_mgcp.c \
rrlp.c \
silent_call.c \
sms_queue.c \
diff --git a/src/libmsc/a_iface.c b/src/libmsc/a_iface.c
index 4892fb8..927efda 100644
--- a/src/libmsc/a_iface.c
+++ b/src/libmsc/a_iface.c
@@ -408,8 +408,8 @@
/* Package RTP-Address data */
memset(_addr_in, 0, sizeof(rtp_addr_in));
rtp_addr_in.sin_family = AF_INET;
-   rtp_addr_in.sin_port = osmo_htons(conn->rtp.port_subscr);
-