Attention is currently required from: pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30982 )

Change subject: bsc_ctrl_commands: Add GET for bts neighbor-list (local bts numbers) ......................................................................

Patch Set 3:

(8 comments)

File src/osmo-bsc/bts_ctrl.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/70cb2cde_af0df228 PS1, 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.

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/22a12acc_b66abba9 PS1, 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.

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/0d818eed_13a2e91c PS1, 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

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/f8c7eb9d_42b3ee81 PS1, 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.

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/473df5e0_1c975160 PS1, 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:

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/ab4544bf_aedc48f6 PS1, 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:

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/58aa382d_968cfd09 PS1, Line 529: 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.

https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/4335fc29_97f70346 PS1, 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