neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-msc/+/37272?usp=email )
Change subject: do not invoke two Assignments (fixup for re-assignment)
......................................................................
do not invoke two Assignments (fixup for re-assignment)
Make sure we wait for Assignment responses before dispatching an
Assignment Request towards RAN.
In particular, when the remote call leg sends its codecs, we may trigger
Re-Assignment to match that codec. Make sure this is done only once:
when receiving another SDP message, wait for the first Assignment.
Implement as an osmo_timer, since there still is no osmo_fsm
implementation for Assignment nor for CC trans FSM. Also it doesn't
belong in the msc_a FSM (it should remain in state COMMUNICATING).
Without this patch, new ttcn3 test TC_lu_and_mo_call_reass_for_mt_codec
sporadically fails, if MNCC with SDP falls in-between Assignment Request
and Assignment Complete.
Related: osmo-msc d767c73a1f93253a54d6a8650a4cf2143353bba0 == I8760feaa8598047369ef8c3ab2673013bac8ac8a
Related: osmo-ttcn3-hacks I402ed0523a2a87b83f29c5577b2c828102005d53
Change-Id: I0f1e1a551aed545b555b9f236fc5967b1e4cc940
---
M include/osmocom/msc/msc_a.h
M src/libmsc/gsm_04_08_cc.c
M src/libmsc/msc_a.c
M src/libmsc/ran_infra.c
4 files changed, 75 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/72/37272/1
diff --git a/include/osmocom/msc/msc_a.h b/include/osmocom/msc/msc_a.h
index a5b624c..d180e38 100644
--- a/include/osmocom/msc/msc_a.h
+++ b/include/osmocom/msc/msc_a.h
@@ -137,6 +137,8 @@
/* There may be up to 7 incoming calls for this subscriber. This is the currently serviced voice call,
* as in, the other person the subscriber is currently talking to. */
struct gsm_trans *active_trans;
+
+ struct osmo_timer_list assignment_request_pending;
} cc;
struct msc_ho_state ho;
diff --git a/src/libmsc/gsm_04_08_cc.c b/src/libmsc/gsm_04_08_cc.c
index 63b1699..fc4f730 100644
--- a/src/libmsc/gsm_04_08_cc.c
+++ b/src/libmsc/gsm_04_08_cc.c
@@ -772,7 +772,8 @@
const struct gsm_mncc_bearer_cap *bcap)
{
struct codec_filter *codecs = &trans->cc.codecs;
- struct call_leg *cl = trans->msc_a ? trans->msc_a->cc.call_leg : NULL;
+ struct msc_a *msc_a = trans->msc_a;
+ struct call_leg *cl = msc_a ? msc_a->cc.call_leg : NULL;
struct rtp_stream *rtp_cn = cl ? cl->rtp[RTP_TO_CN] : NULL;
if (sdp[0]) {
@@ -818,6 +819,16 @@
return;
}
+ if (msc_a && osmo_timer_pending(&msc_a->cc.assignment_request_pending)) {
+ /* Still waiting for an Assignment Response.
+ * For example, when the remote call leg sends some MNCC with SDP with a mismatching codec,
+ * we start Re-Assignment to match that codec: we send an Assignment Request and wait for a response.
+ * When we receive another MNCC with SDP from the remote call leg before this Re-Assignment is
+ * completed, we must not trigger *another* Assignment Request, but instead wait for the Re-Assignment
+ * to come back with a response first. */
+ return;
+ }
+
/* We've already completed Assignment of a voice channel (some time ago), and now the remote side has changed
* to a mismatching codec (list). Try to re-assign this side to a matching codec. */
LOG_TRANS(trans, LOGL_INFO, "Remote call leg mismatches assigned codec: %s\n",
diff --git a/src/libmsc/msc_a.c b/src/libmsc/msc_a.c
index bff5e67..2e88a32 100644
--- a/src/libmsc/msc_a.c
+++ b/src/libmsc/msc_a.c
@@ -673,6 +673,8 @@
rtp_to_ran->codecs_known ? &rtp_to_ran->codecs : NULL, NULL);
}
+static void assignment_request_timeout_cb(void *data);
+
/* The MGW has given us a local IP address for the RAN side. Ready to start the Assignment of a voice channel. */
void msc_a_tx_assignment_cmd(struct msc_a *msc_a)
{
@@ -680,6 +682,19 @@
struct gsm_trans *cc_trans = msc_a->cc.active_trans;
struct gsm0808_channel_type channel_type;
+ /* Do not dispatch another Assignment Command before an earlier assignment is completed. This is a sanity
+ * safeguard, ideally callers should not even invoke this function when an Assignment is already ongoing.
+ * (There is no osmo_fsm for Assignment / the CC trans code; when we refactor that one day, this timer should be
+ * an FSM state.) */
+ if (osmo_timer_pending(&msc_a->cc.assignment_request_pending)) {
+ LOG_MSC_A(msc_a, LOGL_ERROR,
+ "Not transmitting Assignment, still waiting for the response to an earlier Assignment\n");
+ return;
+ }
+ osmo_timer_setup(&msc_a->cc.assignment_request_pending, assignment_request_timeout_cb, msc_a);
+ osmo_timer_schedule(&msc_a->cc.assignment_request_pending,
+ osmo_tdef_get(msc_a->c.ran->tdefs, -37, OSMO_TDEF_S, 10), 0);
+
if (!cc_trans) {
LOG_MSC_A(msc_a, LOGL_ERROR, "No CC transaction active\n");
call_leg_release(msc_a->cc.call_leg);
@@ -943,6 +958,9 @@
vlr_subscr_enable_expire_lu(vsub);
}
+ /* We no longer care about assignment responses. */
+ osmo_timer_del(&msc_a->cc.assignment_request_pending);
+
/* If we're closing in a middle of a trans, we need to clean up */
trans_conn_closed(msc_a);
@@ -1051,6 +1069,7 @@
vsub->msc_conn_ref = NULL;
osmo_timer_del(&msc_a->lu_delay_timer);
+ osmo_timer_del(&msc_a->cc.assignment_request_pending);
}
const struct value_string msc_a_fsm_event_names[] = {
@@ -1479,6 +1498,9 @@
const struct gsm0808_speech_codec *codec_if_known = ac->assignment_complete.codec_present ?
&ac->assignment_complete.codec : NULL;
+ /* Pending assignment has worked out. We're no longer waiting for a response now. */
+ osmo_timer_del(&msc_a->cc.assignment_request_pending);
+
/* For a voice group call, handling is performed by VGCS FSM */
gcc_trans = trans_find_by_type(msc_a, TRANS_GCC);
if (gcc_trans) {
@@ -1582,10 +1604,15 @@
}
}
+/* Invoked when Assignment has failed, either by a failure response, or by timeout. When failing on timeout,
+ * pass af == NULL. */
static void msc_a_up_call_assignment_failure(struct msc_a *msc_a, const struct ran_msg *af)
{
struct gsm_trans *trans;
+ /* Pending assignment has failed. We're no longer waiting for a response now. */
+ osmo_timer_del(&msc_a->cc.assignment_request_pending);
+
/* For a normal voice call, there will be an rtp_stream FSM. */
if (msc_a->cc.call_leg && msc_a->cc.call_leg->rtp[RTP_TO_RAN]) {
LOG_MSC_A(msc_a, LOGL_ERROR, "Assignment Failure, releasing call\n");
@@ -1617,6 +1644,12 @@
msc_a_release_cn(msc_a);
}
+static void assignment_request_timeout_cb(void *data)
+{
+ struct msc_a *msc_a = data;
+ msc_a_up_call_assignment_failure(msc_a, NULL);
+}
+
static void msc_a_up_classmark_update(struct msc_a *msc_a, const struct osmo_gsm48_classmark *classmark,
struct osmo_gsm48_classmark *dst)
{
diff --git a/src/libmsc/ran_infra.c b/src/libmsc/ran_infra.c
index 79de7d2..0656cca 100644
--- a/src/libmsc/ran_infra.c
+++ b/src/libmsc/ran_infra.c
@@ -47,6 +47,8 @@
{ .T = -36, .default_val = 0, .unit = OSMO_TDEF_MS, \
.desc = "Delay connection release after LU. Useful to optimize an SMSC to dispatch " \
"pending messages within the initial connection." }, \
+ { .T = -37, .default_val = 10, \
+ .desc = "Voice channel Assignment sanity timeout, when no response is received (should never happen)." }, \
struct osmo_tdef msc_tdefs_geran[] = {
RAN_TDEFS
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/37272?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I0f1e1a551aed545b555b9f236fc5967b1e4cc940
Gerrit-Change-Number: 37272
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/docker-playground/+/37270?usp=email )
Change subject: ttcn3-s1gw-test: fix using unassigned addr for osmo-s1gw
......................................................................
Patch Set 1:
(1 comment)
File ttcn3-s1gw-test/S1GW_Tests.cfg:
https://gerrit.osmocom.org/c/docker-playground/+/37270/comment/f8ee1812_1c8…
PS1, Line 10: S1GW_Tests.mp_s1gw_enb_ip := "172.18.10.100";
> My understanding is of this patch is that since we asked you drop the multiple interface changes to the docker setup, you decided to move back to use the same IP address for both sides.
Ack.
> It's fine to keep using the same IP address, but if you really need/want to test with multiple addresses, you can do so in the docker setup too, by adding an intermediate .sh script which does "ip addr add A.B.C.D".
This requires `iproute2` package, which is sadly missing in the base `docker` image and in the `debian-bookworm-erlang` we build on top of it. I see you're installing it in `osmo-epdg-master/Dockerfile`, and I could just do the same, but how about installing it in `debian-bookworm-erlang` (in which we already install pkgs like `vim`, `netcat`, `wget`)? Would you CR+ such a patch?
--
To view, visit https://gerrit.osmocom.org/c/docker-playground/+/37270?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: docker-playground
Gerrit-Branch: master
Gerrit-Change-Id: I109a5feaca5acf050008e883cc8b4e1e28beebab
Gerrit-Change-Number: 37270
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 21 Jun 2024 16:42:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/docker-playground/+/37270?usp=email )
Change subject: ttcn3-s1gw-test: fix using unassigned addr for osmo-s1gw
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File ttcn3-s1gw-test/S1GW_Tests.cfg:
https://gerrit.osmocom.org/c/docker-playground/+/37270/comment/91b29da7_b14…
PS1, Line 10: S1GW_Tests.mp_s1gw_enb_ip := "172.18.10.100";
My understanding is of this patch is that since we asked you drop the multiple interface changes to the docker setup, you decided to move back to use the same IP address for both sides.
It's fine to keep using the same IP address, but if you really need/want to test with multiple addresses, you can do so in the docker setup too, by adding an intermediate .sh script which does "ip addr add A.B.C.D". We do that already in several testsuites like PGW, ePDG, Asterisk, etc.
--
To view, visit https://gerrit.osmocom.org/c/docker-playground/+/37270?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: docker-playground
Gerrit-Branch: master
Gerrit-Change-Id: I109a5feaca5acf050008e883cc8b4e1e28beebab
Gerrit-Change-Number: 37270
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 21 Jun 2024 16:29:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/37252?usp=email )
Change subject: asterisk: Implement AMI Action DedicatedBearerStatus
......................................................................
Patch Set 4:
This change is ready for review.
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/37252?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I49c216f8874fe63480414096d9c03a1af00a0fc2
Gerrit-Change-Number: 37252
Gerrit-PatchSet: 4
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Comment-Date: Fri, 21 Jun 2024 16:21:24 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/37260?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
Change subject: asterisk: check Contact attributes on IMS side
......................................................................
asterisk: check Contact attributes on IMS side
Related: SYS#6877
Related: SYS#6981
Change-Id: I866d89ec137d264e257b05226900b744a93c257e
---
M asterisk/Asterisk_Tests.ttcn
M asterisk/IMS_ConnectionHandler.ttcn
2 files changed, 86 insertions(+), 13 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-ttcn3-hacks refs/changes/60/37260/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/37260?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I866d89ec137d264e257b05226900b744a93c257e
Gerrit-Change-Number: 37260
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/37265?usp=email )
Change subject: sctp_proxy: fix no-op 'connecting' state timeout
......................................................................
Patch Set 1:
(2 comments)
File src/sctp_proxy.erl:
https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/37265/comment/fb883a69_31f1…
PS1, Line 85: connecting(enter, OldState,
> tip: I find it useful to prefix these state functions with "st_", this way it's clearer they are par […]
I can rename states, but in a separate patch. This patch is fixing a timer...
https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/37265/comment/0d662bda_29f3…
PS1, Line 90: {next_state, connecting, S#{sock => Sock},
> Reentering the state from withing the enter phase looks a bit werid to me tbh. I think it's totally fine and the proper thing to do to "keep_state" in here [...]
Looks like I did not emphasize this in the commit message clearly enough. I was under the same impression while writing this code, but as it turns out the `state_timeout` is ignored for `keep_state`. I did some experiments and found out that it only works for `next_state`, so this is why I am doing a loop state transition here. The API documentation confirms my discovery.
> The problem I think is that in line 94 connecting_timeout, instead of shutting down you actually need to do "{next_state, connecting, ...}"
No. I do not implement the re-connection logic here, so I am just terminating the FSM. This is intentional and not really related to the problem of timeout never expiring (no mater what value I schedule, even very short ones).
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/37265?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: erlang/osmo-s1gw
Gerrit-Branch: master
Gerrit-Change-Id: I45d874f3f73d5b9a8aa62c8a36e94e51497d6754
Gerrit-Change-Number: 37265
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 21 Jun 2024 14:37:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
