neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 )
Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC ...................................................................... Patch Set 1: Code-Review-1 (2 comments) Found one side effect functional change, and got some style nitpicks https://gerrit.osmocom.org/#/c/15408/1/src/osmo-bsc/bsc_subscr_conn_fsm.c File src/osmo-bsc/bsc_subscr_conn_fsm.c: https://gerrit.osmocom.org/#/c/15408/1/src/osmo-bsc/bsc_subscr_conn_fsm.c@343 PS1, Line 343: if (conn->lchan) { (would prefer the early-exit style; main flow remains undiffed) if (!conn->lchan) { log... state_chg... clear... break; } conn_fsm_state_chg(ST_ACTIVE); https://gerrit.osmocom.org/#/c/15408/1/src/osmo-bsc/bsc_subscr_conn_fsm.c@671 PS1, Line 671: if (!conn->lchan && gscon_is_active(conn)) { Before, when we already were in the ST_CLEARING, we would still re-send the BSSMAP Clear Request again. I am not sure if that is desired, but if that needs to change, that should be a separate patch. It is fine for the BSC to send repeated Clear Requests, they are only polite hints for the MSC. Code design wise, it's not such a good idea to have gscon_is_active() listing all *other* states. If a new state gets added in the future, the author is almost guaranteed to forget to also change that function. Instead I would prefer to more explicitly except only this situation, e.g.: /* If the conn has no lchan anymore, it was released by the BTS and needs to Clear towards MSC. */ if (!conn->lchan) { switch (conn->fi_state) { case ST_INIT: case ST_WAIT_CC: /* The SCCP connection was not yet confirmed by a CC, the BSSAP is not fully established * yet. First wait for the CC, and release in gscon_fsm_wait_cc(). */ break; default: /* Ensure that the FSM is in ST_CLEARING. */ osmo_fsm_inst_state_chg(...ST_CLEARING...) /* fall thru, omit an error log if already in ST_CLEARING */ case ST_CLEARING: /* Request a Clear Command from the MSC. gscon_bssmap_clear([no diff] break; } } Are you sure that ST_INIT should also be in that condition? -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <axilira...@gmail.com> Gerrit-Reviewer: laforge <lafo...@gnumonks.org> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Mon, 16 Sep 2019 15:51:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
