This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
dexter gerrit-no-reply at lists.osmocom.orgdexter has posted comments on this change. ( https://gerrit.osmocom.org/12624 )
Change subject: bsc_vty: add features to disable specific lchans via vty
......................................................................
Patch Set 5:
(9 comments)
> I'm genrally worried when we introduce patches like this.
I get the point, but there are already some other debug commands as well. I have now changed it into a hidden command.
> Specifically regarding this patch: Why introduce a new "disabled" member?
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.
https://gerrit.osmocom.org/#/c/12624/2/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12624/2/include/osmocom/bsc/gsm_data.h@619
PS2, Line 619: bool disabled;
> In general I dislike double-negation booleans, enabled = false is much more straightforward. […]
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.
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.
(This probably requires a discussion on Thursday)
https://gerrit.osmocom.org/#/c/12624/1/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12624/1/include/osmocom/bsc/gsm_data.h@619
PS1, Line 619: disa
> As this flag is related to debug feature, it makes sense to prefix it somehow, e.g. '_shun'...
i have now renamed it to "disabled". I think prefixing it with an underscore is not a good idea.
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/bsc_vty.c
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/bsc_vty.c@1447
PS2, Line 1447: bool all)
> why do you add this 'all' flag?
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.
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/bsc_vty.c@1570
PS2, Line 1570: "Short summary (in use lchans)\n"
> "used lchans" […]
I have split the patch now into two so that we can handle the flag and the show problem separately.
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/bsc_vty.c@1578
PS2, Line 1578: "show lchan all [<0-255>] [<0-255>] [<0-7>] [<0-7>]",
> why pass TS and subslot arguments to "show lchan all"? I thought it shows them all? […]
'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.
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.
I also think its best when we discuss this on thursday.
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/bsc_vty.c@4796
PS2, Line 4796: DEFUN
> if we merge this at all, this should be a hidden command that is now displayed to the normal user.
Done
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/lchan_select.c
File src/osmo-bsc/lchan_select.c:
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/lchan_select.c@83
PS2, Line 83: if (lchan->fi->state == LCHAN_ST_UNUSED && lchan->disabled == false)
> you can re-use LCHAN_ST_BORKEN
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.
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/lchan_select.c@89
PS2, Line 89: LOGPLCHANALLOC("%s ss=%d in type=%s,state=%s %s\n",
> ws
Done
https://gerrit.osmocom.org/#/c/12624/2/src/osmo-bsc/lchan_select.c@94
PS2, Line 94:
> This additional logging would also be unneeded.
I need this to see if the lchan was just "not suitable" (rate/mode does not match) or if it had been intentionally disabled.
--
To view, visit https://gerrit.osmocom.org/12624
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I397e68e26d6a1727890353fa34f4897b54795866
Gerrit-Change-Number: 12624
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Tue, 22 Jan 2019 11:36:06 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190122/298e09a5/attachment.htm>