<p><a href="https://gerrit.osmocom.org/9671">View Change</a></p><p>6 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/include/osmocom/bsc/handover_fsm.h">File include/osmocom/bsc/handover_fsm.h:</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/include/osmocom/bsc/handover_fsm.h@42">Patch Set #6, Line 42:</a> <code style="font-family:monospace,monospace">inter-MO FSM </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what's inter-MO ?  two mobile originated (outbound) hand-overs?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/include/osmocom/bsc/lchan_fsm.h">File include/osmocom/bsc/lchan_fsm.h:</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/include/osmocom/bsc/lchan_fsm.h@53">Patch Set #6, Line 53:</a> <code style="font-family:monospace,monospace">/* FIXME: not yet implemented: Chan Mode Modify */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">does this mean that channel mode modify still bypasses the FSM or that this functionality is missing/broken?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/abis_rsl.c">File src/osmo-bsc/abis_rsl.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/abis_rsl.c@885">Patch Set #6, Line 885:</a> <code style="font-family:monospace,monospace">   if (!lchan->conn) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">when do we expect this? Is it a normal event? should we log it?</p><p style="white-space: pre-wrap; word-wrap: break-word;">The "cause" variable is initialized once to the static value of "0" above and hence could be removed.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/assignment_fsm.c">File src/osmo-bsc/assignment_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/assignment_fsm.c@44">Patch Set #6, Line 44:</a> <code style="font-family:monospace,monospace">#define GET_CONN() \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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:</p><p style="white-space: pre-wrap; word-wrap: break-word;">struct gsm_subscriber_connection *conn = ass_fi_get_conn(fi);</p><p style="white-space: pre-wrap; word-wrap: break-word;">at the caller site, with the ass_fi_get_conn() then doing the dereference and ASSERTs and returning the pointer.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The ass_fi_geet_conn() name is just an example, not set in stone.</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/assignment_fsm.c@48">Patch Set #6, Line 48:</a> <code style="font-family:monospace,monospace">struct state_timeout assignment_fsm_timeouts[32] = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">is either of tose possible here: const? static?</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/assignment_fsm.c@294">Patch Set #6, Line 294:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">    static bool g_initialized = false;<br>    if (!g_initialized) {<br>         OSMO_ASSERT(osmo_fsm_register(&assignment_fsm) == 0);<br>             g_initialized = true;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm much more in favor of an explicit registration somewhere. But not critical.</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 16:18:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>