<p><a href="https://gerrit.osmocom.org/9671">View Change</a></p><p>26 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c">File src/osmo-bsc/bsc_vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c@a1240">Patch Set #6, Line 1240:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">was it moved to libosmgsm?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c@1727">Patch Set #6, Line 1727:</a> <code style="font-family:monospace,monospace">DEFUN(handover_ext_cgi,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we should probably have a FIXME here</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c">File src/osmo-bsc/gsm_data.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1536">Patch Set #6, Line 1536:</a> <code style="font-family:monospace,monospace">static enum gsm0808_permitted_speech audio_support_to_gsm88(const struct gsm_audio_support *audio)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">static helper functions here and below also don't seem gsm_data.[ch] related and should be moved wherever their sole caller is moved.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1653">Patch Set #6, Line 1653:</a> <code style="font-family:monospace,monospace">int bsc_match_codec_pref(enum gsm48_chan_mode *chan_mode,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1708">Patch Set #6, Line 1708:</a> <code style="font-family:monospace,monospace">bool sockaddr_to_str_and_uint(char *rtp_addr, size_t rtp_addr_len, uint16_t *rtp_port,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1715">Patch Set #6, Line 1715:</a> <code style="font-family:monospace,monospace">osmo_ntohs</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c">File src/osmo-bsc/handover_decision.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c@203">Patch Set #6, Line 203:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision_2.c">File src/osmo-bsc/handover_decision_2.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision_2.c@722">Patch Set #6, Line 722:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why that separate block?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c">File src/osmo-bsc/handover_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@141">Patch Set #6, Line 141:</a> <code style="font-family:monospace,monospace">#define GET_CONN() \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comment about hiding variable declarations in macros</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@249">Patch Set #6, Line 249:</a> <code style="font-family:monospace,monospace">    static bool g_initialized = false;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comment about avoiding this automatic registration</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@644">Patch Set #6, Line 644:</a> <code style="font-family:monospace,monospace">               return result_counter_INTER_BSC_HO_MT(result);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">two return statements in one case?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_logic.c">File src/osmo-bsc/handover_logic.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_logic.c@99">Patch Set #6, Line 99:</a> <code style="font-family:monospace,monospace">int handover_count(struct gsm_bts *bts, int ho_scopes)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">might make sense to prefix symbol name with bts_ ?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c">File src/osmo-bsc/lchan_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c@41">Patch Set #6, Line 41:</a> <code style="font-family:monospace,monospace">#define GET_LCHAN() \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comment about hiding variable declaration in macros</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c@326">Patch Set #6, Line 326:</a> <code style="font-family:monospace,monospace">   static bool g_initialized = false;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comment about automatic vs. explicit registration</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c">File src/osmo-bsc/lchan_select.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c@1">Patch Set #6, Line 1:</a> <code style="font-family:monospace,monospace">/* Select a suitable lchan from a given cell.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c">File src/osmo-bsc/mgw_endpoint_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@188">Patch Set #6, Line 188:</a> <code style="font-family:monospace,monospace">     if (!g_initialized) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous code about automatic vs. explicit registration</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@196">Patch Set #6, Line 196:</a> <code style="font-family:monospace,monospace">#define GET_MGWEP() \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comment about hiding vatiable declarations in macros</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@372">Patch Set #6, Line 372:</a> <code style="font-family:monospace,monospace">struct state_timeout mgwep_fsm_timeouts[32] = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">do we need those non-static?  can they be const?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@682">Patch Set #6, Line 682:</a> <code style="font-family:monospace,monospace">int mgwep_fsm_timer_cb(struct osmo_fsm_inst *fi)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">do we need to export this symbol? is it used by anyting but the timer_cb below?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c">File src/osmo-bsc/osmo_bsc_api.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c@189">Patch Set #6, Line 189:</a> <code style="font-family:monospace,monospace">no-one is using this</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is odd? any service we don't know / support should be rejected. how does the code do this?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c">File src/osmo-bsc/osmo_bsc_bssap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@757">Patch Set #6, Line 757:</a> <code style="font-family:monospace,monospace">         LOGP(DMSC, LOGL_ERROR, "Received Handover Command, but no handover was requested");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">might make sense to log "conn" context here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@767">Patch Set #6, Line 767:</a> <code style="font-family:monospace,monospace">           LOGP(DMSC, LOGL_ERROR, "Mandatory IE not present: Layer 3 Information\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">might make sense to log "conn" context here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@772">Patch Set #6, Line 772:</a> <code style="font-family:monospace,monospace">    {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comment about those extra blocks rather than declaring the variable at the top of the function.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c">File src/osmo-bsc/timeslot_fsm.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@40">Patch Set #6, Line 40:</a> <code style="font-family:monospace,monospace">#define GET_TS(fi, TS) \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comments about hiding variable declarations in macros</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@44">Patch Set #6, Line 44:</a> <code style="font-family:monospace,monospace">#define GET_BTS_TS(fi, BTS, TS) \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comments about hiding variable declarations in macros</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@57">Patch Set #6, Line 57:</a> <code style="font-family:monospace,monospace">       if (!g_initialized) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">see previous comments about explicit registration</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/9671">change 9671</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/9671"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I82e3f918295daa83274a4cf803f046979f284366 </div>
<div style="display:none"> Gerrit-Change-Number: 9671 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 25 Jun 2018 19:11:49 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>