<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/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/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1331">Patch Set #1, Line 1331:</a> <code style="font-family:monospace,monospace"> bo</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think all other members are self-explanatory, but this one maybe could deserver a one-line comment […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">'releasing' with 'e'</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1402">Patch Set #1, Line 1402:</a> <code style="font-family:monospace,monospace">/* Find any busy TCH/H or TCH/F lchan, prefer established and old lchans */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not preferring "old" lchans. The order of the timeslots is no good indicator for the duration of the call, especially when a cell is continuously in use. The first used lchan can often be the most recently established, if another call ended shortly before that and released the first slot, which was then rapidly taken for the next call.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't this also favor actually unused lchans?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't this also favor an lchan that is currently being released, over an established one?</p><p style="white-space: pre-wrap; word-wrap: break-word;">And, what if by coincidence all lchans are currently being established, then this should probably still return one of them and abort the establishment to service the emergency call instead?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/src/osmo-bsc/abis_rsl.c@1447">Patch Set #1, Line 1447:</a> <code style="font-family:monospace,monospace">static int handle_emergency_call(struct chan_rqd *rqd)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the logic in this function confuses me:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">- the name suggests that it handles a call, but then when an lchan is available it just exits.<br> (maybe it should be called: 'force_free_lchan_for_emergency'?)<br>- the 'relasing' case sounds like an emergency call is releasing, but still the function looks for an lchan first.<br>- the negative return code in the end seems to not be an error?</pre></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793/1/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/osmo-bsc/+/19793/1/src/osmo-bsc/lchan_select.c@254">Patch Set #1, Line 254:</a> <code style="font-family:monospace,monospace">lchan_select_avail</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">naming. We are not selecting a channel, as we are not returning it, or performing any action on it. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">'lchan_avail_by_type()', i.e. just s/select/avail from above function name.</p><p style="white-space: pre-wrap; word-wrap: break-word;">A more elegant arrangement would be this:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct gsm_lchan *lchan_avail_by_type(bts, type)<br> {<br> [...most of current lchan_select_by_type() function body...]<br> return lchan;<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct gsm_lchan *lchan_select_by_type(bts, type)<br> {<br> lchan = lchan_avail_by_type(bts, type);<br> if (lchan)<br> lchan->type = type;<br> [...]<br> return lchan;<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">i.e. no third static function is necessary, because the 'select' is just a continuation of the 'avail'</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/19793">change 19793</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/c/osmo-bsc/+/19793"/><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-Change-Id: If8651265928797dbda9f528b544931dcfa4a0b36 </div>
<div style="display:none"> Gerrit-Change-Number: 19793 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 24 Aug 2020 11:55:11 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>