neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659 )
Change subject: Add vty interface ...................................................................... Patch Set 2: (5 comments) https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/Makefile.am File src/osmo-bsc-nat/Makefile.am: https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/Makefile.am@31 PS2, Line 31: vty.c \ I know it's a different source tree, but personally, I prefer if the file name reflects the context: mainly because it is easier to ctags-jump to bsc_nat_vty.c than skip through all the matches for vty.c across all the osmo git source trees... Just mentioning it, it's fine if you prefer this way. https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/logging.c File src/osmo-bsc-nat/logging.c: https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/logging.c@25 PS2, Line 25: [DMAIN] = { "DMAIN" seems odd. Usually main() just does fprintf(stderr) or LOGP(DLGLOBAL). So semantically "main" is not really a subsystem IMO? https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/logging.c@38 PS2, Line 38: (i usually just put these things in main.c, don't see a reason for a separate logging.c file, especially since that noises up ctags by shadowing the libosmocore logging.c file ... up to you) https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/main.c File src/osmo-bsc-nat/main.c: https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/main.c@47 PS2, Line 47: .go_parent_cb = bsc_nat_vty_go_parent, (can drop this, see other comment) https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/vty.c File src/osmo-bsc-nat/vty.c: https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/vty.c@31 PS2, Line 31: int bsc_nat_vty_go_parent(struct vty *vty) you no longer need to define a go_parent or is_config_node callback. Those were needed for the legacy VTY node handling. The new/current vty code implicitly figures out what nodes nest in what other nodes, and also which vty->index object to point to when exiting a child node. The only reason to implement a go_parent cb is if you want specific actions to happen when the VTY node is exited; imagine that you want to start up or shut down a component when the user is done configuring a node. Usually that's a bad UI design decision though. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc-nat Gerrit-Branch: master Gerrit-Change-Id: I6f2d8d1d62b97be471ebcf1bb14aac0b895833d1 Gerrit-Change-Number: 26659 Gerrit-PatchSet: 2 Gerrit-Owner: osmith <osm...@sysmocom.de> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: neels <nhofm...@sysmocom.de> Gerrit-Comment-Date: Tue, 28 Dec 2021 13:08:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
