Change in ...osmo-sgsn[master]: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
pespin has submitted this change and it was merged. (
https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 )
Change subject: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
..
gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
For new readers it's very confusing why PMM states and MM states are in
the same enum, but handled with different functions, and sometimes
called one right after the other with different enums. Calling them when
on a different ran_type makes the function early return, so let's better
conditionally call the function to make it clear in the flow when the
function is expected to do something.
Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73
---
M src/gprs/gprs_gmm.c
1 file changed, 27 insertions(+), 14 deletions(-)
Approvals:
laforge: Looks good to me, but someone else must approve
osmith: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/src/gprs/gprs_gmm.c b/src/gprs/gprs_gmm.c
index 95a1842..6c87032 100644
--- a/src/gprs/gprs_gmm.c
+++ b/src/gprs/gprs_gmm.c
@@ -138,8 +138,7 @@
static void mmctx_set_pmm_state(struct sgsn_mm_ctx *ctx, enum gprs_pmm_state
state)
{
- if (ctx->ran_type != MM_CTX_T_UTRAN_Iu)
- return;
+ OSMO_ASSERT(ctx->ran_type == MM_CTX_T_UTRAN_Iu);
if (ctx->pmm_state == state)
return;
@@ -164,8 +163,7 @@
static void mmctx_set_mm_state(struct sgsn_mm_ctx *ctx, enum gprs_pmm_state
state)
{
- if (ctx->ran_type != MM_CTX_T_GERAN_Gb)
- return;
+ OSMO_ASSERT(ctx->ran_type == MM_CTX_T_GERAN_Gb);
if (ctx->pmm_state == state)
return;
@@ -326,8 +324,15 @@
/* Mark MM state as deregistered */
ctx->gmm_state = GMM_DEREGISTERED;
- mmctx_set_pmm_state(ctx, PMM_DETACHED);
- mmctx_set_mm_state(ctx, MM_IDLE);
+
+ switch(ctx->ran_type) {
+ case MM_CTX_T_UTRAN_Iu:
+ mmctx_set_pmm_state(ctx, PMM_DETACHED);
+ break;
+ case MM_CTX_T_GERAN_Gb:
+ mmctx_set_mm_state(ctx, MM_IDLE);
+ break;
+ }
sgsn_mm_ctx_cleanup_free(ctx);
}
@@ -2078,16 +2083,20 @@
mmctx->t3350_mode = GMM_T3350_MODE_NONE;
mmctx->p_tmsi_old = 0;
mmctx->pending_req = 0;
- if (mmctx->ran_type == MM_CTX_T_GERAN_Gb) {
+ mmctx->gmm_state = GMM_REGISTERED_NORMAL;
+ switch(mmctx->ran_type) {
+ case MM_CTX_T_UTRAN_Iu:
+ mmctx_set_pmm_state(mmctx, PMM_CONNECTED);
+ break;
+ case MM_CTX_T_GERAN_Gb:
/* Unassign the old TLLI */
mmctx->gb.tlli = mmctx->gb.tlli_new;
gprs_llme_copy_key(mmctx, mmctx->gb.llme);
gprs_llgmm_assign(mmctx->gb.llme, TLLI_UNASSIGNED,
mmctx->gb.tlli_new);
+ mmctx_set_mm_state(mmctx, MM_READY);
+ break;
}
- mmctx->gmm_state = GMM_REGISTERED_NORMAL;
- mmctx_set_pmm_state(mmctx, PMM_CONNECTED);
- mmctx_set_mm_state(mmctx, MM_READY);
rc = 0;
osmo_fsm_inst_dispatch(mmctx->gmm_att_req.fsm,
E_ATTACH_COMPLETE_RECV, 0);
@@ -2104,15 +2113,19 @@
mmctx->t3350_mode = GMM_T3350_MODE_NONE;
mmctx->p_tmsi_old = 0;
mmctx->pending_req = 0;
- if (mmctx->ran_type == MM_CTX_T_GERAN_Gb) {
+ mmctx->gmm_state = GMM_REGISTERED_NORMAL;
+ switch(mmctx->ran_type) {
+ case MM_CTX_T_UTRAN_Iu:
+ mmctx_set_pmm_state(mmctx, PMM_CONNECTED);
+ break;
+ case MM_CTX_T_GERAN_Gb:
/* Unassign the old TLLI */
mmctx->gb.tlli = mmctx->gb.tlli_new;
gprs_llgmm_assign(mmctx->gb.llme, TLLI_UNASSIGNED,
mmctx->gb.tlli_new);
+ mmctx_set_mm_state(mmctx, MM_READY);
+ break;
}
- mmctx->gmm_state = GMM_REGISTERED_NORMAL;
- mmctx_set_pmm_state(mmctx, PMM_CONNECTED);
- mmctx_set_mm_state(mmctx, MM_READY);
rc = 0;
memset(_data, 0, sizeof(sig_data));
--
To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15186
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73
Gerrit-Change-Number: 15186
Gerrit-PatchSet: 4
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Reviewer: pespin
Change in ...osmo-sgsn[master]: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 ) Change subject: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type .. Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73 Gerrit-Change-Number: 15186 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-CC: neels Gerrit-Comment-Date: Tue, 20 Aug 2019 10:21:56 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 ) Change subject: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73 Gerrit-Change-Number: 15186 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-CC: neels Gerrit-Comment-Date: Sat, 17 Aug 2019 01:23:32 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 to look at the new patch set (#3). Change subject: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type .. gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type For new readers it's very confusing why PMM states and MM states are in the same enum, but handled with different functions, and sometimes called one right after the other with different enums. Calling them when on a different ran_type makes the function early return, so let's better conditionally call the function to make it clear in the flow when the function is expected to do something. Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73 --- M src/gprs/gprs_gmm.c 1 file changed, 27 insertions(+), 14 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/86/15186/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73 Gerrit-Change-Number: 15186 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-CC: neels Gerrit-MessageType: newpatchset
Change in ...osmo-sgsn[master]: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
neels has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 )
Change subject: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
..
Patch Set 2:
(7 comments)
I'm advocating for OSMO_ASSERTS(); they would probably give us more SGSN
crashes, but then we stand a chance of finding bugs related to a subscriber
switching RAN types. The aim could also be to not crash and instead log errors
and detach the subscriber, not so sure.
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c
File src/gprs/gprs_gmm.c:
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@140
PS2, Line 140: {
Vadim's idea seems very good, and I would add:
Place an OSMO_ASSERT for matching RAN type to catch all bugs that mix and mess
it up.
(Am not aware of the callers but would assume no mismatching RAN type state
should ever be passed to this.)
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@153
PS2, Line 153: case PMM_CONNECTED:
(while at it we could lose this 'case', or even make the entire switch into an
if())
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@204
PS2, Line 204: mmctx_set_pmm_state(mm, PMM_IDLE);
how could a RANAP_IU_EVENT come in on a ran_type != MM_CTX_T_UTRAN_Iu? I guess
that should be an OSMO_ASSERT()?
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@1096
PS2, Line 1096: case GSM48_MT_GMM_SERVICE_REQ:
again this looks like it is clearly related to Iu and ran_type should always be
UTRAN_Iu?
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@2084
PS2, Line 2084: mmctx->gmm_state = GMM_REGISTERED_NORMAL;
I know this was in the code before, but it seems to overwrite the previous
state. Both RAN types set the state after this, so this is redundant and may
confuse logging?
(It says "Changing state from X to Y", where the X is here always
GMM_REGISTERED_NORMAL, although the state would have been something else before
overwriting?)
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@2097
PS2, Line 2097: case MM_CTX_T_GERAN_Iu:
(instead of all of these dead switch cases, maybe we should rather "#if 0" away
the unused enum member)
https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@2116
PS2, Line 2116: mmctx->gmm_state = GMM_REGISTERED_NORMAL;
(again weird overwriting of the current state, like above)
--
To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15186
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73
Gerrit-Change-Number: 15186
Gerrit-PatchSet: 2
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria
Gerrit-CC: neels
Gerrit-Comment-Date: Wed, 14 Aug 2019 23:39:43 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 ) Change subject: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c File src/gprs/gprs_gmm.c: https://gerrit.osmocom.org/#/c/15186/2/src/gprs/gprs_gmm.c@a139 PS2, Line 139: Maybe we should rather keep only one function? The only difference between the both is that we call mmctx_change_gtpu_endpoints_to_sgsn() if (ctx->ran_type == MM_CTX_T_UTRAN_Iu && state == PMM_IDLE). -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15186 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73 Gerrit-Change-Number: 15186 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-CC: fixeria Gerrit-Comment-Date: Wed, 14 Aug 2019 23:05:40 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
pespin has uploaded this change for review. (
https://gerrit.osmocom.org/c/osmo-sgsn/+/15186
Change subject: gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
..
gprs_gmm.c: Call mmctx_set_(p)mm_state only on related ran_type
For new readers it's very confusing why PMM states and MM states are in
the same enum, but handled with different functions, and sometimes
called one right after the other with different enums. Calling them when
on a different ran_type makes the function early return, so let's better
conditionally call the function to make it clear in the flow when the
function is expected to do something.
Change-Id: I65ad9e180177bc9fc7c4a037cd85cfe33b161f73
---
M src/gprs/gprs_gmm.c
1 file changed, 34 insertions(+), 18 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/86/15186/1
diff --git a/src/gprs/gprs_gmm.c b/src/gprs/gprs_gmm.c
index 61c5908..1e586c2 100644
--- a/src/gprs/gprs_gmm.c
+++ b/src/gprs/gprs_gmm.c
@@ -138,9 +138,6 @@
static void mmctx_set_pmm_state(struct sgsn_mm_ctx *ctx, enum gprs_pmm_state
state)
{
- if (ctx->ran_type != MM_CTX_T_UTRAN_Iu)
- return;
-
if (ctx->pmm_state == state)
return;
@@ -164,9 +161,6 @@
static void mmctx_set_mm_state(struct sgsn_mm_ctx *ctx, enum gprs_pmm_state
state)
{
- if (ctx->ran_type != MM_CTX_T_GERAN_Gb)
- return;
-
if (ctx->pmm_state == state)
return;
@@ -206,7 +200,7 @@
else
LOGMMCTXP(LOGL_INFO, mm, "IU release for UE conn
0x%x\n",
ctx->conn_id);
- if (mm && mm->pmm_state == PMM_CONNECTED)
+ if (mm && mm->ran_type == MM_CTX_T_UTRAN_Iu && mm->pmm_state ==
PMM_CONNECTED)
mmctx_set_pmm_state(mm, PMM_IDLE);
rc = 0;
break;
@@ -326,8 +320,17 @@
/* Mark MM state as deregistered */
ctx->gmm_state = GMM_DEREGISTERED;
- mmctx_set_pmm_state(ctx, PMM_DETACHED);
- mmctx_set_mm_state(ctx, MM_IDLE);
+
+ switch(ctx->ran_type) {
+ case MM_CTX_T_UTRAN_Iu:
+ mmctx_set_pmm_state(ctx, PMM_DETACHED);
+ break;
+ case MM_CTX_T_GERAN_Gb:
+ mmctx_set_mm_state(ctx, MM_IDLE);
+ break;
+ case MM_CTX_T_GERAN_Iu:
+ break;
+ }
sgsn_mm_ctx_cleanup_free(ctx);
}
@@ -1092,7 +1095,8 @@
#ifdef BUILD_IU
case GSM48_MT_GMM_SERVICE_REQ:
ctx->pending_req = 0;
- mmctx_set_pmm_state(ctx, PMM_CONNECTED);
+ if (ctx->ran_type == MM_CTX_T_UTRAN_Iu)
+ mmctx_set_pmm_state(ctx, PMM_CONNECTED);
rc = gsm48_tx_gmm_service_ack(ctx);
if (ctx->iu.service.type != GPRS_SERVICE_T_SIGNALLING)
@@ -2073,16 +2077,22 @@
mmctx->t3350_mode = GMM_T3350_MODE_NONE;
mmctx->p_tmsi_old = 0;
mmctx->pending_req = 0;
- if (mmctx->ran_type == MM_CTX_T_GERAN_Gb) {
+ mmctx->gmm_state = GMM_REGISTERED_NORMAL;
+ switch(mmctx->ran_type) {
+ case MM_CTX_T_UTRAN_Iu:
+ mmctx_set_pmm_state(mmctx, PMM_CONNECTED);
+ break;
+ case MM_CTX_T_GERAN_Gb:
/* Unassign the old TLLI */
mmctx->gb.tlli = mmctx->gb.tlli_new;
gprs_llme_copy_key(mmctx, mmctx->gb.llme);
gprs_llgmm_assign(mmctx->gb.llme, TLLI_UNASSIGNED,
mmctx->gb.tlli_new);
+ mmctx_set_mm_state(mmctx, MM_READY);
+ break;
+ case MM_CTX_T_GERAN_Iu:
+ break;
}
- mmctx->gmm_state = GMM_REGISTERED_NORMAL;
- mmctx_set_pmm_state(mmctx, PMM_CONNECTED);
- mmctx_set_mm_state(mmctx, MM_READY);
rc = 0;
osmo_fsm_inst_dispatch(mmctx->gmm_att_req.fsm,
E_ATTACH_COMPLETE_RECV, 0);
@@ -2099,15 +2109,21 @@
mmctx->t3350_mode = GMM_T3350_MODE_NONE;
mmctx->p_tmsi_old = 0;
mmctx->pending_req = 0;
- if (mmctx->ran_type == MM_CTX_T_GERAN_Gb) {
+ mmctx->gmm_state = GMM_REGISTERED_NORMAL;
+ switch(mmctx->ran_type) {
+ case MM_CTX_T_UTRAN_Iu:
+ mmctx_set_pmm_state(mmctx, PMM_CONNECTED);
+ break;
+ case MM_CTX_T_GERAN_Gb:
/* Unassign the old TLLI */
mmctx->gb.tlli = mmctx->gb.tlli_new;
gprs_llgmm_assign(mmctx->gb.llme, TLLI_UNASSIGNED,
mmctx->gb.tlli_new);
+
