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: (26 comments) https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c@a1240 PS6, Line 1240: was it moved to libosmgsm? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c@1727 PS6, Line 1727: DEFUN(handover_ext_cgi, we should probably have a FIXME here https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c File src/osmo-bsc/gsm_data.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1536 PS6, Line 1536: static enum gsm0808_permitted_speech audio_support_to_gsm88(const struct gsm_audio_support *audio) static helper functions here and below also don't seem gsm_data.[ch] related and should be moved wherever their sole caller is moved. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1653 PS6, Line 1653: int bsc_match_codec_pref(enum gsm48_chan_mode *chan_mode, this is also probably not really right here in gsm_data.c. IT doesn't work with lchan/ts/trx/bts type structures but purely on other data. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1708 PS6, Line 1708: bool sockaddr_to_str_and_uint(char *rtp_addr, size_t rtp_addr_len, uint16_t *rtp_port, this is not something we'd traditionally have in gsm_data.c - also it seems like it's a rather generic function. candidate for libosmocore? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1715 PS6, Line 1715: osmo_ntohs we normally only use osmo_ntohs() in situations where it's not IP/socket related and hence htons() may not be available. If you're working on a port, ntohs/htons/etc. are available. But anyway, we can probably ignore it. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c File src/osmo-bsc/handover_decision.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c@203 PS6, Line 203: { style-wise, I would generally prefer the variable declaration at the top of the function, rather than those separate blocks. This applies to all of the code. I don't think we've been doing this in osmocom so far (I don't recall it?). Why now? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision_2.c File src/osmo-bsc/handover_decision_2.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision_2.c@722 PS6, Line 722: { why that separate block? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c File src/osmo-bsc/handover_fsm.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@141 PS6, Line 141: #define GET_CONN() \ see previous comment about hiding variable declarations in macros https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@249 PS6, Line 249: static bool g_initialized = false; see previous comment about avoiding this automatic registration https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@644 PS6, Line 644: return result_counter_INTER_BSC_HO_MT(result); two return statements in one case? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_logic.c File src/osmo-bsc/handover_logic.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_logic.c@99 PS6, Line 99: int handover_count(struct gsm_bts *bts, int ho_scopes) might make sense to prefix symbol name with bts_ ? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c File src/osmo-bsc/lchan_fsm.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c@41 PS6, Line 41: #define GET_LCHAN() \ see previous comment about hiding variable declaration in macros https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c@326 PS6, Line 326: static bool g_initialized = false; see previous comment about automatic vs. explicit registration https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c File src/osmo-bsc/lchan_select.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c@1 PS6, Line 1: /* Select a suitable lchan from a given cell. I guess this is code we traditionally would have had in chan_alloc.c? Any particular rationale to have a new file and not use chan_alloc.c? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c File src/osmo-bsc/mgw_endpoint_fsm.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@188 PS6, Line 188: if (!g_initialized) { see previous code about automatic vs. explicit registration https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@196 PS6, Line 196: #define GET_MGWEP() \ see previous comment about hiding vatiable declarations in macros https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@372 PS6, Line 372: struct state_timeout mgwep_fsm_timeouts[32] = { do we need those non-static? can they be const? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@682 PS6, Line 682: int mgwep_fsm_timer_cb(struct osmo_fsm_inst *fi) do we need to export this symbol? is it used by anyting but the timer_cb below? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c File src/osmo-bsc/osmo_bsc_api.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c@189 PS6, Line 189: no-one is using this this is odd? any service we don't know / support should be rejected. how does the code do this? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c File src/osmo-bsc/osmo_bsc_bssap.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@757 PS6, Line 757: LOGP(DMSC, LOGL_ERROR, "Received Handover Command, but no handover was requested"); might make sense to log "conn" context here? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@767 PS6, Line 767: LOGP(DMSC, LOGL_ERROR, "Mandatory IE not present: Layer 3 Information\n"); might make sense to log "conn" context here? https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@772 PS6, Line 772: { see previous comment about those extra blocks rather than declaring the variable at the top of the function. https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c File src/osmo-bsc/timeslot_fsm.c: https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@40 PS6, Line 40: #define GET_TS(fi, TS) \ see previous comments about hiding variable declarations in macros https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@44 PS6, Line 44: #define GET_BTS_TS(fi, BTS, TS) \ see previous comments about hiding variable declarations in macros https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@57 PS6, Line 57: if (!g_initialized) { see previous comments about explicit registration -- 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-CC: Harald Welte <[email protected]> Gerrit-Comment-Date: Mon, 25 Jun 2018 19:11:49 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
