Change in osmo-bsc[master]: lchan_fsm: safer 'concluded' flag

[Neels Hofmeyr]

Neels Hofmeyr has uploaded this change for review. ( https://gerrit.osmocom.org/10638


Change subject: lchan_fsm: safer 'concluded' flag
......................................................................

lchan_fsm: safer 'concluded' flag

The flag lchan->activate.concluded prevents entering the
lchan_on_fully_established()/lchan_on_activation_failure() more than once. So
far it was checked by callers, instead place in the functions themselves.

There is a potential functional change here, since during lchan_fail(), the
concluded flag was set only after entering lchan_on_activation_failure(). Now
it is already set at the start and prevents multiple re-entry beyond doubt.

This patch is not a result of an actual observed faulty behavior, just from
reading the code and seeing the slight loophole.

Change-Id: I0c906438b05442d66255203eb816453b7193a6d8
---
M include/osmocom/bsc/gsm_data.h
M src/osmo-bsc/lchan_fsm.c
2 files changed, 13 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/38/10638/1

diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index b827d0a..d6e8954 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -505,7 +505,10 @@
 	struct {
 		enum lchan_activate_mode activ_for;
 		bool activ_ack; /*< true as soon as RSL Chan Activ Ack is received */
-		bool concluded; /*< true as soon as LCHAN_ST_ESTABLISHED is reached */
+		/*! This flag ensures that when an lchan activation has succeeded, and we have already
+		 * sent ACKs like Immediate Assignment or BSSMAP Assignment Complete, and if other errors
+		 * occur later, e.g. during release, that we don't send a NACK out of context. */
+		bool concluded;
 		bool requires_voice_stream;
 		bool wait_before_switching_rtp; /*< true = requires LCHAN_EV_READY_TO_SWITCH_RTP */
 		uint16_t msc_assigned_cic;
diff --git a/src/osmo-bsc/lchan_fsm.c b/src/osmo-bsc/lchan_fsm.c
index e856fd0..13ae7ad 100644
--- a/src/osmo-bsc/lchan_fsm.c
+++ b/src/osmo-bsc/lchan_fsm.c
@@ -88,6 +88,10 @@
 					 struct gsm_subscriber_connection *for_conn,
 					 const char *file, int line)
 {
+	if (lchan->activate.concluded)
+		return;
+	lchan->activate.concluded = true;
+
 	switch (activ_for) {
 
 	case FOR_MS_CHANNEL_REQUEST:
@@ -135,6 +139,10 @@
 
 static void lchan_on_fully_established(struct gsm_lchan *lchan)
 {
+	if (lchan->activate.concluded)
+		return;
+	lchan->activate.concluded = true;
+
 	switch (lchan->activate.activ_for) {
 	case FOR_MS_CHANNEL_REQUEST:
 		/* No signalling to do here, MS is free to use the channel, and should go on to connect
@@ -222,9 +230,7 @@
 		lchan_set_last_error(_lchan, "lchan %s in state %s: " fmt, \
 				     _lchan->activate.concluded ? "failure" : "allocation failed", \
 				     osmo_fsm_state_name(fsm, state_was), ## args); \
-		if (!_lchan->activate.concluded) \
-			lchan_on_activation_failure(_lchan, _lchan->activate.activ_for, _lchan->conn); \
-		_lchan->activate.concluded = true; \
+		lchan_on_activation_failure(_lchan, _lchan->activate.activ_for, _lchan->conn); \
 		if (fi->state != state_chg) \
 			lchan_fsm_state_chg(state_chg); \
 		else \
@@ -751,10 +757,6 @@
 		return;
 	}
 
-	/* This flag ensures that when an lchan activation has succeeded, and we have already sent ACKs
-	 * like Immediate Assignment or BSSMAP Assignment Complete, and if then, way later, some other
-	 * error occurs, e.g. during release, that we don't send a NACK out of context. */
-	lchan->activate.concluded = true;
 	lchan_on_fully_established(lchan);
 }
 

-- 
To view, visit https://gerrit.osmocom.org/10638
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0c906438b05442d66255203eb816453b7193a6d8
Gerrit-Change-Number: 10638
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180827/263e8622/attachment.htm>