Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...

[neels]

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 <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at gnumonks.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 16 Sep 2019 15:51:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190916/c80d79a5/attachment.htm>