<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I'm genrally worried when we introduce patches like this. </p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I get the point, but there are already some other debug commands as well. I have now changed it into a hidden command.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Specifically regarding this patch:  Why introduce a new "disabled" member?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">My idea was to stay away from the lchan FSM as far as possible. I am using this for test in order to simulate an occupied lchan. I fear that when I mess with the FSM, I might introduce bugs or change the behaviour in the way that the tests become less realistic. Basically I want to trick the channel allocator into thinking that the LCHAN is not suitable and the rest should work normally.</p><p><a href="https://gerrit.osmocom.org/12624">View Change</a></p><p>9 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">In general I dislike double-negation booleans, enabled = false is much more straightforward. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I intentionally use a negated boolean because when the struct is initialized with zeros it will be set to false automatically. Apart from that the normal state of the disabled flag is false.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I decided not to mess with the lchan state machine or with the broken (borken?) state. I want to keep this feature away from the normal code.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(This probably requires a discussion on Thursday)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12624/1/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/1/include/osmocom/bsc/gsm_data.h@619">Patch Set #1, Line 619:</a> <code style="font-family:monospace,monospace">disa</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">As this flag is related to debug feature, it makes sense to prefix it somehow, e.g. '_shun'...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i have now renamed it to "disabled". I think prefixing it with an underscore is not a good idea.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why do you add this 'all' flag?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">There are two views: One only shows the currently used lchans and one shows all available. For normal inspection it may make sens to hide the lchans that are currently not in use. For debugging i may want to see all of them, regardless of their state.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"used lchans" […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have split the patch now into two so that we can handle the flag and the show problem separately.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why pass TS and subslot arguments to "show lchan all"? I thought it shows them all? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">'show lchan summary' shows only the lchans that are in use. 'show lchan summary 1 2' shows all lchans that are in use on bts 1, trx 2. If no lchan is in use, nothing is displayed. This command is already current master.</p><p style="white-space: pre-wrap; word-wrap: break-word;">And yes, I want to see all lchans, also the disabled ones. So I added 'show lchan all' that works exactly the same like the already existing 'show lchan summary' Maybe we can add some option to the existing 'show lchan summary' somehow, but I have no Idea how to do that.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I also think its best when we discuss this on thursday.</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@4796">Patch Set #2, Line 4796:</a> <code style="font-family:monospace,monospace">DEFUN</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">if we merge this at all, this should be a hidden command that is now displayed to the normal user.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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;">you can re-use LCHAN_ST_BORKEN</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Messing with the FSM is what I am trying to avoid. I fear that messing with the FSM might change the behavior to much. I am using this to assist integration tests and I want to simulate an occupied lchan as close as possible.</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/lchan_select.c@89">Patch Set #2, Line 89:</a> <code style="font-family:monospace,monospace">                 LOGPLCHANALLOC("%s ss=%d in type=%s,state=%s %s\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ws</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/lchan_select.c@94">Patch Set #2, Line 94:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This additional logging would also be unneeded.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I need this to see if the lchan was just "not suitable" (rate/mode does not match) or if it had been intentionally disabled.</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: 5 </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-Reviewer: dexter <pmaier@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: Tue, 22 Jan 2019 11:36:06 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>