Change in osmo-bsc[master]: assignment_fsm: tweak state transitions (prep for reassignment)

[neels]

neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/24361 )

Change subject: assignment_fsm: tweak state transitions (prep for reassignment)
......................................................................

assignment_fsm: tweak state transitions (prep for reassignment)

It is better design to take state change actions in the onenter
function, instead of triggering a state change and then calling
lchan_mode_modify() or lchan_activate(). The reason is that in
principle, any state change may cause error handling to abort and
deallocate FSMs.

This is also preparation for reassignment to a specific lchan in an
upcoming patch.

Related: SYS#5315 OS#4940 OS#3277
Change-Id: I9a2e7eefd4196b80671311e5dfd275893ec0e275
---
M src/osmo-bsc/assignment_fsm.c
1 file changed, 29 insertions(+), 16 deletions(-)

Approvals:
  Jenkins Builder: Verified
  laforge: Looks good to me, but someone else must approve
  fixeria: Looks good to me, approved



diff --git a/src/osmo-bsc/assignment_fsm.c b/src/osmo-bsc/assignment_fsm.c
index f294473..83d2917 100644
--- a/src/osmo-bsc/assignment_fsm.c
+++ b/src/osmo-bsc/assignment_fsm.c
@@ -447,8 +447,6 @@
 		[CH_RATE_FULL] = "FR",
 	};
 	struct osmo_fsm_inst *fi;
-	struct lchan_activate_info activ_info;
-	struct lchan_modify_info modif_info;
 	int i;
 
 	OSMO_ASSERT(conn);
@@ -510,16 +508,7 @@
 			       gsm48_chan_mode_name(conn->assignment.selected_ch_mode_rate.chan_mode),
 			       gsm_lchan_name(conn->lchan));
 
-		modif_info = (struct lchan_modify_info){
-			.modify_for = MODIFY_FOR_ASSIGNMENT,
-			.ch_mode_rate = conn->assignment.selected_ch_mode_rate,
-			.requires_voice_stream = conn->assignment.requires_voice_stream,
-			.msc_assigned_cic = req->msc_assigned_cic,
-		};
-
-		if (assignment_fsm_state_chg(ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED))
-			return;
-		lchan_mode_modify(conn->lchan, &modif_info);
+		assignment_fsm_state_chg(ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED);
 		return;
 	}
 
@@ -564,7 +553,13 @@
 		       req->use_osmux ? "yes" : "no");
 
 	assignment_fsm_state_chg(ASSIGNMENT_ST_WAIT_LCHAN_ACTIVE);
-	activ_info = (struct lchan_activate_info){
+}
+
+static void assignment_fsm_wait_lchan_active_onenter(struct osmo_fsm_inst *fi, uint32_t prev_state)
+{
+	struct gsm_subscriber_connection *conn = assignment_fi_conn(fi);
+	struct assignment_request *req = &conn->assignment.req;
+	struct lchan_activate_info activ_info = {
 		.activ_for = ACTIVATE_FOR_ASSIGNMENT,
 		.for_conn = conn,
 		.ch_mode_rate = conn->assignment.selected_ch_mode_rate,
@@ -578,14 +573,17 @@
 	lchan_activate(conn->assignment.new_lchan, &activ_info);
 }
 
-static void assignment_fsm_wait_lchan(struct osmo_fsm_inst *fi, uint32_t event, void *data)
+static void assignment_fsm_wait_lchan_active(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	struct gsm_subscriber_connection *conn = assignment_fi_conn(fi);
 	switch (event) {
 
 	case ASSIGNMENT_EV_LCHAN_ACTIVE:
-		if (data != conn->assignment.new_lchan)
+		if (data != conn->assignment.new_lchan) {
+			LOG_ASSIGNMENT(conn, LOGL_ERROR, "Some unrelated lchan was activated, ignoring: %s\n",
+				       gsm_lchan_name(data));
 			return;
+		}
 
 		/* The TS may have changed its pchan_is */
 		assignment_fsm_update_id(conn);
@@ -736,6 +734,19 @@
 	}
 }
 
+static void assignment_fsm_wait_lchan_modified_onenter(struct osmo_fsm_inst *fi, uint32_t prev_state)
+{
+	struct gsm_subscriber_connection *conn = assignment_fi_conn(fi);
+	struct assignment_request *req = &conn->assignment.req;
+	struct lchan_modify_info modif_info = {
+		.modify_for = MODIFY_FOR_ASSIGNMENT,
+		.ch_mode_rate = conn->assignment.selected_ch_mode_rate,
+		.requires_voice_stream = conn->assignment.requires_voice_stream,
+		.msc_assigned_cic = req->msc_assigned_cic,
+	};
+	lchan_mode_modify(conn->lchan, &modif_info);
+}
+
 static void assignment_fsm_wait_lchan_modified(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	switch (event) {
@@ -754,7 +765,8 @@
 static const struct osmo_fsm_state assignment_fsm_states[] = {
 	[ASSIGNMENT_ST_WAIT_LCHAN_ACTIVE] = {
 		.name = "WAIT_LCHAN_ACTIVE",
-		.action = assignment_fsm_wait_lchan,
+		.onenter = assignment_fsm_wait_lchan_active_onenter,
+		.action = assignment_fsm_wait_lchan_active,
 		.in_event_mask = 0
 			| S(ASSIGNMENT_EV_LCHAN_ACTIVE)
 			,
@@ -799,6 +811,7 @@
 	},
 	[ASSIGNMENT_ST_WAIT_LCHAN_MODIFIED] = {
 		.name = "WAIT_LCHAN_MODIFIED",
+		.onenter = assignment_fsm_wait_lchan_modified_onenter,
 		.action = assignment_fsm_wait_lchan_modified,
 		.in_event_mask = 0
 			| S(ASSIGNMENT_EV_LCHAN_MODIFIED)

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/24361
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I9a2e7eefd4196b80671311e5dfd275893ec0e275
Gerrit-Change-Number: 24361
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210601/e933b6dc/attachment.htm>