<p>Patch set 2:<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/12624">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12624/2/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.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/12624/2/include/osmocom/bsc/gsm_data.h@619">Patch Set #2, Line 619:</a> <code style="font-family:monospace,monospace"> bool disabled;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">In general I dislike double-negation booleans, enabled = false is much more straightforward.<br>Vadim has a point, I would prefer to not add a separate flag and instead put an lchan in LCHAN_ST_BORKEN state.<br>It will then remain unusable until manual intervention.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This could also make more sense on the VTY API: a command like "lchan set broken" is obviously a feature no operator would misunderstand as something useful besides a broken BTS, and maybe in that case laforge would agree to keep it in the vty reference manual?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12624/2/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/12624/2/src/osmo-bsc/bsc_vty.c@1447">Patch Set #2, Line 1447:</a> <code style="font-family:monospace,monospace"> bool all)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why do you add this 'all' flag?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/bsc_vty.c@1570">Patch Set #2, Line 1570:</a> <code style="font-family:monospace,monospace"> "Short summary (in use lchans)\n"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"used lchans"</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is unrelated to a debugging flag</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/bsc_vty.c@1578">Patch Set #2, Line 1578:</a> <code style="font-family:monospace,monospace"> "show lchan all [<0-255>] [<0-255>] [<0-7>] [<0-7>]",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why pass TS and subslot arguments to "show lchan all"? I thought it shows them all?</p><p style="white-space: pre-wrap; word-wrap: break-word;">So far the semantics are that 'show lchan summary 1 2' shows BTS 1 TS 2, and 'show lchan summary' shows all. So I oppose adding this command.</p><p style="white-space: pre-wrap; word-wrap: break-word;">What is the goal -- do you want to achieve printing out all disabled lchans? then use that flag to trigger printing? Maybe we can also talk about this in person to figure out how to best achieve what you need.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12624/2/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/12624/2/src/osmo-bsc/lchan_select.c@83">Patch Set #2, Line 83:</a> <code style="font-family:monospace,monospace">if (lchan->fi->state == LCHAN_ST_UNUSED && lchan->disabled == false)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">At this point I've got an idea: what if we introduce a special FSM state for that? For example, LCHA […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">you can re-use LCHAN_ST_BORKEN</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12624">change 12624</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/12624"/><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: I397e68e26d6a1727890353fa34f4897b54795866 </div>
<div style="display:none"> Gerrit-Change-Number: 12624 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </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-CC: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 21 Jan 2019 14:15:21 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>