<p><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/Makefile.am">File src/osmo-bsc-nat/Makefile.am:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/Makefile.am@31">Patch Set #2, Line 31:</a> <code style="font-family:monospace,monospace">     vty.c \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/logging.c">File src/osmo-bsc-nat/logging.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/logging.c@25">Patch Set #2, Line 25:</a> <code style="font-family:monospace,monospace">    [DMAIN] = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"DMAIN" seems odd. Usually main() just does fprintf(stderr) or LOGP(DLGLOBAL).<br>So semantically "main" is not really a subsystem IMO?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/logging.c@38">Patch Set #2, Line 38:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(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)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/main.c">File src/osmo-bsc-nat/main.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/main.c@47">Patch Set #2, Line 47:</a> <code style="font-family:monospace,monospace">    .go_parent_cb   = bsc_nat_vty_go_parent,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(can drop this, see other comment)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/vty.c">File src/osmo-bsc-nat/vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659/2/src/osmo-bsc-nat/vty.c@31">Patch Set #2, Line 31:</a> <code style="font-family:monospace,monospace">int bsc_nat_vty_go_parent(struct vty *vty)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659">change 26659</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-bsc-nat/+/26659"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc-nat </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I6f2d8d1d62b97be471ebcf1bb14aac0b895833d1 </div>
<div style="display:none"> Gerrit-Change-Number: 26659 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 28 Dec 2021 13:08:03 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>