Attention is currently required from: pespin.
8 comments:
File src/osmo-bsc/bts_ctrl.c:
Patch Set #1, Line 587: OSMO_STRBUF_PRINTF(*csv, "%" PRIu8 ",", n->bts_nr);
You actually need to print bts->nr, not bts->bts_nr. […]
The tests give exactly what is asked for in https://projects.sysmocom.de/issues/6287#note-4
Pretty sure I also saw that `bts->bts_nr` is the struct member those numbers are parsed into somewhere in the code.
Patch Set #1, Line 607: if (csv.chars_needed >= csv.len) { /* Just to be save, although this is very unlikely */
It's impossible that this is triggered. […]
Well if it's better to optimize downwards, we can even go down to 512 bytes (short calculation):
(a BTS can have itself as a neighbor in the config for whatever reason, so)
max. 256 BTS neighbors comma-separated, that makes 255 commas max, add the trailing '\0': 256 bytes.
10 of those numbers (0...9) are 1-digit numbers: 256 + 10 = 266 bytes
90 of those numbers are 2-digit numbers (10...99): 266 + 90 = 356 bytes
255 - 100 + 1 = 156 are 3-digit numbers (100...255): 356 + 156 = 512 bytes
The test config now has a double entry for neighbor bts to also implicitly test against double BTS num entries, as well as an entry with a too high number (1000) as a kind of assertion that nothing weird gets parsed.
Patch Set #1, Line 614: write_neighbors(&bts->neighbors, &csv);
since you no longer need this function twice, just drop te function and inline it's just a loop.
Done
Patch Set #1, Line 617: cmd->reply = "No BTS neighbors configured through BTS number";
return empty string here, it's way easier to parse by clients.
Okay. It's what I did at first, but in between I got confused, because of how we parse `cmd->reply` as `r['value']` and I'm not sure anymore what happens to `cmd->value`, but it isn't parsed as `r['value']`, so that led to some weird results along the way.
Think I also saw some other replies that were human-language sentences in existing code.
Patch Set #1, Line 630: CTRL_CMD_DEFINE_RO(bts_neighbor_list, "neighbor-list");
My understanding is that we already have "bts.N.neighbor-bts.add" and "bts.N.neighbor-bts. […]
Thanks, initially I simply went by the command name in the request, but this makes more sense indeed. I moved the command to the source file with the 'neighbor-bts.*' commands as well
File tests/ctrl/osmo-bsc-neighbor-list.cfg:
Patch Set #1, Line 1: network
Please if possible drop all config lines which are not really needed/used by the test.
Would you help me with this? I don't know much about any of this as of now and reading a lot of documentation just in order to be able to eliminate parameters here wouldn't be very time efficient (and I don't think it'd be sustainably acquired knowledge, either).
Going for trial and error to figure out what is dispensable for this to work seems like a very inefficient use of time, too. Using the minimal config file and just adding the neighbor bts as a line didn't work out.
(This file is deleted now, I eventually opted to extend the existing file `osmo-bsc-neigh-tests.cfg`)
File tests/ctrl_test_runner.py:
I submitted this along with the rest of it, because I read we are following the Kernel coding guidelines (at least to some degree), where I read this (https://docs.kernel.org/process/4.Coding.html):
But pure coding style fixes are seen as noise by the development community; they tend to get a chilly reception. So this type of patch is best avoided. It is natural to fix the style of a piece of code while working on it for other reasons, but coding style changes should not be made for their own sake.
Doesn't matter for this patch anymore, because the file actually isn't consistent w.r.t. whether or not there's an empty line behind the `def` statements. But it would be good to know whether or not we diverge from Kernel coding guidelines here.
Patch Set #1, Line 580: self.assertEqual(r['value'], 'No BTS neighbors configured through BTS number')
this makes it difficult for the client. Simply return an empty string in osmo-bsc in this case.
Done
To view, visit change 30982. To unsubscribe, or for help writing mail filters, visit settings.