Hi all,
Attached patch adds vty command for setting Access control classes (GSM 02.11 Section 4 Access control) in RACH Control Parameters (GSM 04.08 Section 10.5.2.29 RACH Control Parameters).
Ivan Kluchnikov wrote:
+DEFUN(cfg_bts_rach_ac_classes, cfg_bts_rach_ac_classes_cmd,
"rach access control classes (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) x (0|1) (0|1) (0|1) (0|1) (0|1)",RACH_STR"Access control classes\n""Access control classes\n""Access control classes\n""Access is NOT barred for ACC 0\n""Access is barred for ACC 0\n""Access is NOT barred for ACC 1\n"
..
The command looks pretty horrible. Do you think that there could be a better user interface than a single command with 16 parameters?
I'd suggest something like: rach access control class (0..15) (0|1)
//Peter
Peter Stuge wrote:
The command looks pretty horrible. Do you think that there could be a better user interface than a single command with 16 parameters?
I'd suggest something like: rach access control class (0..15) (0|1)
yes, it makes sense. also it would make sense to actually write the access classes to vty config, if "show running-configuration" or "write" is used. (see config_write_bts_single())
since "0" (not barred) is the default, i would suggest to only write the barred ("1") classes to config. this way the config is kept slim.
On Fri, Jul 12, 2013 at 10:48 PM, Andreas Eversberg andreas@eversberg.eu wrote:
also it would make sense to actually write the access classes to vty config, if "show running-configuration" or "write" is used. (see config_write_bts_single())
Yes, this is a good point. Should be fixed.
since "0" (not barred) is the default, i would suggest to only write the barred ("1") classes to config. this way the config is kept slim.
See my e-mail to Peter. Looks like a matter of taste.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
On Fri, Jul 12, 2013 at 10:28 PM, Peter Stuge peter@stuge.se wrote:
Ivan Kluchnikov wrote:
+DEFUN(cfg_bts_rach_ac_classes, cfg_bts_rach_ac_classes_cmd,
"rach access control classes (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) (0|1) x (0|1) (0|1) (0|1) (0|1) (0|1)",RACH_STR"Access control classes\n""Access control classes\n""Access control classes\n""Access is NOT barred for ACC 0\n""Access is barred for ACC 0\n""Access is NOT barred for ACC 1\n"..
The command looks pretty horrible. Do you think that there could be a better user interface than a single command with 16 parameters?
I'd suggest something like: rach access control class (0..15) (0|1)
I like our way more, as it's a single line vs many-many lines. A single line doesn't occupy so much screen space and is easier to manipulate from VTY or over a slow satellite connection (happen frequently here). Also it gives you the whole picture in a single glance.
Does it sound like a valid reason?
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
Alexander Chemeris wrote:
The command looks pretty horrible. Do you think that there could be a better user interface than a single command with 16 parameters?
I'd suggest something like: rach access control class (0..15) (0|1)
I like our way more, as it's a single line vs many-many lines. A single line doesn't occupy so much screen space and is easier to manipulate from VTY or over a slow satellite connection (happen frequently here). Also it gives you the whole picture in a single glance.
Does it sound like a valid reason?
No not the least. Compare:
rach access control classes 0 1 1 0 0 1 0 0 1 0 0 0 1 1 0
with:
rach access control class 1 1 rach access control class 2 1 rach access control class 5 0 rach access control class 5 1 rach access control class 8 1 rach access control class 13 1 rach access control class 14 1
I know which I find clearer, and easier to manipulate with a minimum of unwanted side effects.
"The connectivity to my sites suck" really can't motivate such a bad user interface. Use mosh and/or train operators to deal with higher latency connections.
//Peter
PS. Did you notice the error in my 'class' commands? PPS. Did you notice the error in your 'classes' command?
Peter Stuge wrote:
PS. Did you notice the error in my 'class' commands? PPS. Did you notice the error in your 'classes' command?
I hope this made my point clear.
There is another option though; you could allow to specify ranges.
rach access control classes 1-5,8-10,12,15
...it just takes a few sscanf() calls in a loop.
(No 0|1 at all, the given classes are allowed, others are barred.)
//Peter
Peter Stuge wrote:
Peter Stuge wrote:
PS. Did you notice the error in my 'class' commands? PPS. Did you notice the error in your 'classes' command?
I hope this made my point clear.
There is another option though; you could allow to specify ranges.
rach access control classes 1-5,8-10,12,15
I think it should still be called "rach access control class" however.
//Peter
On Fri, Jul 12, 2013 at 11:35 PM, Peter Stuge peter@stuge.se wrote:
There is another option though; you could allow to specify ranges.
rach access control classes 1-5,8-10,12,15
...it just takes a few sscanf() calls in a loop.
(No 0|1 at all, the given classes are allowed, others are barred.)
That would be a nice option. I don't think we have time to implement parsing of that right now, but if someone makes this, we definitely vote for this way. Otherwise we'll stick to the way discussed above.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
On Fri, Jul 12, 2013 at 11:27 PM, Peter Stuge peter@stuge.se wrote:
Compare:
rach access control classes 0 1 1 0 0 1 0 0 1 0 0 0 1 1 0
with:
rach access control class 1 1 rach access control class 2 1 rach access control class 5 0 rach access control class 5 1 rach access control class 8 1 rach access control class 13 1 rach access control class 14 1
I know which I find clearer, and easier to manipulate with a minimum of unwanted side effects.
PS. Did you notice the error in my 'class' commands? PPS. Did you notice the error in your 'classes' command?
Ok, now it makes sense.
I think we could even drop the last (0|1) and make it like that: rach access control class barred 1 rach access control class barred 2 rach access control class barred 5 (default being not barred)
OTOH I like the idea that you could change value without removing a whole string, so may be make it even more user-friendly: rach access control class 1 barred rach access control class 2 barred rach access control class 5 allowed
The idea is that "1" meaning "barred" is counter-intuitive and may lead to wrong setting. Even I will forget abut the trick in a couple of months.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
On Fri, Jul 12, 2013 at 09:08:42PM +0400, Ivan Kluchnikov wrote:
Hi all,
Attached patch adds vty command for setting Access control classes (GSM 02.11 Section 4 Access control) in RACH Control Parameters (GSM 04.08 Section 10.5.2.29 RACH Control Parameters).
Hi,
mutt doesn't allow me to comment on the patch (for inline patches this always works, for some attachments it does work too.. not for yours).
My comments:
* Peter is right, and use "barred"|"allowed" as well instead of 0,1. * The 'current' VTY rule is that unless there is a common prefix use '-' in the command. So it would be 'rach access-control-class" right now until we have someone else at 'access-control'.. * The code is missing a 'write' implementation. * You might want to write a VTY test[1] to test if the command is taking effect on the SIs. You will need to create a subclass for the NITB. * Please take a look at the kernel coding style * Great to have this feature, for things like Fusion this is a useful addition! * Great you patched pySIM for the ACC support.
thanks holger
[1] http://cgit.osmocom.org/openbsc/tree/openbsc/tests/vty_test_runner.py
Hi Holger,
On Sat, Jul 13, 2013 at 9:58 AM, Holger Hans Peter Freyther holger@freyther.de wrote:
mutt doesn't allow me to comment on the patch (for inline patches this always works, for some attachments it does work too.. not for yours).
Probably because it's base64 encoded.
- The 'current' VTY rule is that unless there is a common prefix use '-' in the command. So it would be 'rach access-control-class" right now until we have someone else at 'access-control'..
Huh, is that a new rule? I see commands like "rach max transmission", "rach tx integer" and so on which do not use "-".
Thanks for your other comments. We'll look into updating the patch.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
On Sat, Jul 13, 2013 at 10:50:40AM +0400, Alexander Chemeris wrote:
- The 'current' VTY rule is that unless there is a common prefix use '-' in the command. So it would be 'rach access-control-class" right now until we have someone else at 'access-control'..
Huh, is that a new rule?
Not exactly, the rule has been there before we used the VTY.. I just never used cisco stuff before. ;)
I see commands like "rach max transmission", "rach tx integer" and so on which do not use "-".
Yeah, we have plenty of violations (and I don't think it is worth fixing). For new ones just try to get it right.
On Sat, Jul 13, 2013 at 1:46 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sat, Jul 13, 2013 at 10:50:40AM +0400, Alexander Chemeris wrote:
- The 'current' VTY rule is that unless there is a common prefix use '-' in the command. So it would be 'rach access-control-class" right now until we have someone else at 'access-control'..
Huh, is that a new rule?
Not exactly, the rule has been there before we used the VTY.. I just never used cisco stuff before. ;)
Me either :)
I see commands like "rach max transmission", "rach tx integer" and so on which do not use "-".
Yeah, we have plenty of violations (and I don't think it is worth fixing). For new ones just try to get it right.
Ok.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
Hi all,
This is new version of patch for review and merge:
- more user-friendly interface for command: rach access-control-class 2 barred rach access-control-class 11 allowed
- added 'write' implementation
2013/7/13 Alexander Chemeris alexander.chemeris@gmail.com
On Sat, Jul 13, 2013 at 1:46 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sat, Jul 13, 2013 at 10:50:40AM +0400, Alexander Chemeris wrote:
- The 'current' VTY rule is that unless there is a common prefix use '-' in the command. So it would be 'rach access-control-class"
right
now until we have someone else at 'access-control'..
Huh, is that a new rule?
Not exactly, the rule has been there before we used the VTY.. I just never used cisco stuff before. ;)
Me either :)
I see commands like "rach max transmission", "rach tx integer" and so on which do not use "-".
Yeah, we have plenty of violations (and I don't think it is worth fixing). For new ones just try to get it right.
Ok.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
On Thu, Jul 25, 2013 at 06:19:24PM +0400, Ivan Kluchnikov wrote:
- added 'write' implementation
please write a VTY test for this routine. It is like 20 lines of python and will make sure that bitrot is detected early (a perfect way to secure your time investment).
+ if ((bts->si_common.rach_control.t3) != 0) ...
+ if ((bts->si_common.rach_control.t2 & 0xfb) != 0) ...
you want to avoid executing the for loop?
holger
2013/7/25 Holger Hans Peter Freyther holger@freyther.de
On Thu, Jul 25, 2013 at 06:19:24PM +0400, Ivan Kluchnikov wrote:
- added 'write' implementation
please write a VTY test for this routine. It is like 20 lines of python and will make sure that bitrot is detected early (a perfect way to secure your time investment).
Yes, I agree, tests are very important, I will try to write one for this command.
if ((bts->si_common.rach_control.t3) != 0)...
if ((bts->si_common.rach_control.t2 & 0xfb) != 0)...
you want to avoid executing the for loop?
Yes, do you think it is redundant part of the code?
holger
On Thu, Jul 25, 2013 at 07:21:55PM +0400, Ivan Kluchnikov wrote:
2013/7/25 Holger Hans Peter Freyther holger@freyther.de
Yes, I agree, tests are very important, I will try to write one for this command.
To help you I created the below snippet. You will just need to write a new test method in there to do config and show configure. E.g. you can issue all bar commands and verify that it shows up in the write terminal command, then allow them again and see that it goes away.
diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py index 3aa40f4..a3e29d3 100644 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -55,6 +55,19 @@ class TestVTYBase(unittest.TestCase): self.vty = None osmoutil.end_proc(self.proc)
+class TestVTYNITB(TestVTYBase): + + def vty_command(self): + return ["./src/osmo-nitb/osmo-nitb", "-c", + "doc/examples/osmo-nitb/nanobts/openbsc.cfg"] + + def vty_app(self): + return (4242, "./src/osmo-nitb/osmo-nitb", "OpenBSC", "nitb") + + def testShowNetwork(self): + res = self.vty.command("show network") + print res +
class TestVTYNAT(TestVTYBase):
@@ -138,6 +151,7 @@ if __name__ == '__main__': os.chdir(workdir) print "Running tests for specific VTY commands" suite = unittest.TestSuite() + suite.addTest(unittest.TestLoader().loadTestsFromTestCase(TestVTYNITB)) add_nat_test(suite, workdir) res = unittest.TextTestRunner(verbosity=verbose_level).run(suite) sys.exit(len(res.errors) + len(res.failures))
Ivan Kluchnikov wrote:
This is new version of patch for review and merge:
- more user-friendly interface for command: rach access-control-class 2 barred rach access-control-class 11 allowed
Yes, much better than before, but perhaps spend the half hour it takes to write that %d-%d parser using sscanf?
+++ b/openbsc/src/libbsc/bsc_vty.c
..
@@ -2059,6 +2067,51 @@ DEFUN(cfg_bts_rach_ec_allowed, cfg_bts_rach_ec_allowed_cmd, return CMD_SUCCESS; }
+DEFUN(cfg_bts_rach_ac_class, cfg_bts_rach_ac_class_cmd,
"rach access-control-class (0|1|2|3|4|5|6|7|8|9|11|12|13|14|15) (barred|allowed)",RACH_STR"Set access control class\n""Access control class 0\n""Access control class 1\n""Access control class 2\n""Access control class 3\n""Access control class 4\n""Access control class 5\n""Access control class 6\n""Access control class 7\n""Access control class 8\n""Access control class 9\n""Access control class 11 for PLMN use\n""Access control class 12 for security services\n""Access control class 13 for public utilities (e.g. water/gas suppliers)\n""Access control class 14 for emergency services\n""Access control class 15 for PLMN staff\n""barred to use access control class\n""allowed to use access control class\n")+{
Is that long array of strings correct?
//Peter
2013/7/25 Peter Stuge peter@stuge.se
Ivan Kluchnikov wrote:
This is new version of patch for review and merge:
- more user-friendly interface for command: rach access-control-class 2 barred rach access-control-class 11 allowed
Yes, much better than before, but perhaps spend the half hour it takes to write that %d-%d parser using sscanf?
You are welcome :) If you implement %d-%d parser, I will change style of this VTY command.
+++ b/openbsc/src/libbsc/bsc_vty.c
..
@@ -2059,6 +2067,51 @@ DEFUN(cfg_bts_rach_ec_allowed,
cfg_bts_rach_ec_allowed_cmd,
return CMD_SUCCESS;}
+DEFUN(cfg_bts_rach_ac_class, cfg_bts_rach_ac_class_cmd,
"rach access-control-class (0|1|2|3|4|5|6|7|8|9|11|12|13|14|15)(barred|allowed)",
RACH_STR"Set access control class\n""Access control class 0\n""Access control class 1\n""Access control class 2\n""Access control class 3\n""Access control class 4\n""Access control class 5\n""Access control class 6\n""Access control class 7\n""Access control class 8\n""Access control class 9\n""Access control class 11 for PLMN use\n""Access control class 12 for security services\n""Access control class 13 for public utilities (e.g. water/gassuppliers)\n"
"Access control class 14 for emergency services\n""Access control class 15 for PLMN staff\n""barred to use access control class\n""allowed to use access control class\n")+{
Is that long array of strings correct?
I think, yes, because it works. Do you know other way to do it?
//Peter
Ivan Kluchnikov wrote:
Ivan Kluchnikov wrote:
This is new version of patch for review and merge:
- more user-friendly interface for command: rach access-control-class 2 barred rach access-control-class 11 allowed
Yes, much better than before, but perhaps spend the half hour it takes to write that %d-%d parser using sscanf?
You are welcome :) If you implement %d-%d parser, I will change style of this VTY command.
Here you go.
//Peter
Peter Stuge wrote:
if (',' == *p) p++;Here it should actually also throw parse error if there is any other character than comma or end-of-input:
And there was an off-by-one error for the upper bound of a range.
I think I've caught all the issues in this version of the file. :)
//Peter
Yes, the next step is to integrate this code into osmocore VTY code.
2013/7/25 Peter Stuge peter@stuge.se
Peter Stuge wrote:
if (',' == *p) p++;Here it should actually also throw parse error if there is any other character than comma or end-of-input:
And there was an off-by-one error for the upper bound of a range.
I think I've caught all the issues in this version of the file. :)
//Peter
-
Alexander Chemeris -
Andreas Eversberg -
Holger Hans Peter Freyther -
Ivan Kluchnikov -
Peter Stuge