Change in osmo-bsc[master]: large refactoring: use FSMs for lchans; add inter-BSC HO

[Harald Welte]

Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/9671 )

Change subject: large refactoring: use FSMs for lchans; add inter-BSC HO
......................................................................


Patch Set 6:

(6 comments)

https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/handover_fsm.h
File include/osmocom/bsc/handover_fsm.h:

https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/handover_fsm.h@42
PS6, Line 42: inter-MO FSM 
what's inter-MO ?  two mobile originated (outbound) hand-overs?


https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/lchan_fsm.h
File include/osmocom/bsc/lchan_fsm.h:

https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/lchan_fsm.h@53
PS6, Line 53: /* FIXME: not yet implemented: Chan Mode Modify */
does this mean that channel mode modify still bypasses the FSM or that this functionality is missing/broken?


https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/abis_rsl.c
File src/osmo-bsc/abis_rsl.c:

https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/abis_rsl.c@885
PS6, Line 885: 	if (!lchan->conn) {
when do we expect this? Is it a normal event? should we log it?

The "cause" variable is initialized once to the static value of "0" above and hence could be removed.


https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c
File src/osmo-bsc/assignment_fsm.c:

https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c@44
PS6, Line 44: #define GET_CONN() \
I'm sorry, but I don't like the idea of hiding a variable declaration in a macro.  I don't think we have any precedent for that.  I think This could well become:

struct gsm_subscriber_connection *conn = ass_fi_get_conn(fi);

at the caller site, with the ass_fi_get_conn() then doing the dereference and ASSERTs and returning the pointer.

The ass_fi_geet_conn() name is just an example, not set in stone.


https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c@48
PS6, Line 48: struct state_timeout assignment_fsm_timeouts[32] = {
is either of tose possible here: const? static?


https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c@294
PS6, Line 294: 	static bool g_initialized = false;
             : 	if (!g_initialized) {
             : 		OSMO_ASSERT(osmo_fsm_register(&assignment_fsm) == 0);
             : 		g_initialized = true;
             : 	}
I'm much more in favor of an explicit registration somewhere. But not critical.



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

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e3f918295daa83274a4cf803f046979f284366
Gerrit-Change-Number: 9671
Gerrit-PatchSet: 6
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-Comment-Date: Mon, 25 Jun 2018 16:18:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180625/c2c9c959/attachment.htm>