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); 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 */ 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); Done https://gerrit.osmocom.org/c/osmo-bsc/+/30982/comment/f8c7eb9d_42b3ee81 PS1, Line 617: cmd->reply = "No BTS neighbors configured through BTS number"; 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"); 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 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): 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') Done -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30982 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I79aeffd93089086f57c66787fe20b439a4d8b6b4 Gerrit-Change-Number: 30982 Gerrit-PatchSet: 3 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Sat, 21 Jan 2023 21:53:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment