<p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24600">View Change</a></p><p>8 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/+/24600/1/doc/manuals/chapters/control.adoc">File doc/manuals/chapters/control.adoc:</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/+/24600/1/doc/manuals/chapters/control.adoc@41">Patch Set #1, Line 41:</a> <code style="font-family:monospace,monospace">|handover_ho_active|RW|No|"0","1","default"|Enable/disable handover.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">be aware that (almost?) all handover, handover1 and handover2 vty commands exist on the 'network' node *as well as* on each 'bts' node. (These are overlayed; a BTS that has no value set uses the 'network' node's defaults)</p><p style="white-space: pre-wrap; word-wrap: break-word;">So to be complete there should also be bts.N.handover* nodes</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c">File src/osmo-bsc/handover_ctrl.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/+/24600/1/src/osmo-bsc/handover_ctrl.c@33">Patch Set #1, Line 33:</a> <code style="font-family:monospace,monospace">bool verify_vty_cmd_arg(void *ctx, const char *range, const char *value)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ack</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i understand: the macros that were invented for VTY are now used for CTRL. Now we have argument descriptions in string literals, like "<0-255>" or "fr|hr", because those can be conveniently dropped in place in the vty code, and we get validation for free from vty.c.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For CTRL arguments, there is no implicit argument range checking. pmaier's implementation parses the string literal that is describing the argument in vty syntax, in order to validate that the CTRL user passed valid arguments. Essentially this is a duplication of the argument validation code in vty/command.c cmd_match().</p><p style="white-space: pre-wrap; word-wrap: break-word;">I would prefer to expose that cmd_match() as public API and use that here instead of re-implementing the parser. Can you try whether that works out?</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/+/24600/1/src/osmo-bsc/handover_ctrl.c@80">Patch Set #1, Line 80:</a> <code style="font-family:monospace,monospace">CTRL_CMD_DEFINE(NAME, "handover_"#NAME); \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The vty command name is in VTY_CMD;<br>NAME is the name used in C code.<br>The difference by some examples:</p><p style="white-space: pre-wrap; word-wrap: break-word;">NAME VTY0 VTY_CMD<br>algorithm "" handover algorithm<br>hodec1_pwr_interval handover1 power budget interval<br>hodec2_rxlev_avg_win handover2 window rxlev averaging</p><p style="white-space: pre-wrap; word-wrap: break-word;">it's a neat trick to just use the C name on the CTRL interface, because then we are guaranteed to use an unambiguous keyword without special characters.</p><p style="white-space: pre-wrap; word-wrap: break-word;">However what I don't like that much is that now the naming on the CTRL is quite different from on the VTY.<br>The discrepancy in naming I think came from legacy code and it was acceptable as long as it was hidden in the code.<br>But on the public CTRL, it would be nicer to use CTRL cmd names that are exactly like VTY_CMD, just with underscores for spaces.<br>I see these ways:</p><p style="white-space: pre-wrap; word-wrap: break-word;">1) adjust the #NAME strings to more exactly match the VTY command string.<br>for that we need to also replace the strings in the handover C code (where osmo-bsc queries the config).</p><p style="white-space: pre-wrap; word-wrap: break-word;">2) add another argument to HO_CFG_ONE_MEMBER that is exactly the CTRL command.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I prefer 1).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also I would like to see scoping as: handover.* , handover1.* and handover2.*</p><p style="white-space: pre-wrap; word-wrap: break-word;">So for example, instead of</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> handover_hodec2_rxlev_avg_win</pre><p style="white-space: pre-wrap; word-wrap: break-word;">I would like to see</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> handover2.window_rxlev_averaging<br> bts.N.handover2.window_rxlev_averaging</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@88">Patch Set #1, Line 88:</a> <code style="font-family:monospace,monospace"> cmd->reply = talloc_asprintf(cmd, VTY_WRITE_FMT, VTY_WRITE_CONV(val)); \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why are we using VTY stuff here? Are these fmts safe for ctrl interface? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">the HO_CFG_ONE_MEMBER() has arguments called VTY_WRITE_FMT and VTY_WRITE_CONV.<br>That's because the macro scheme was invented to avoid code dup and copy paste errors in the handover VTY configuration.<br>Now the same are being used for CTRL.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We maybe could rename the macro arguments to be more general, like just drop all the VTY_ prefixes across handover_cfg.*? <br>Anyway, that would be cosmetic noise, not really harmful if we keep the names and drop a comment above for others wondering the same thing</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/+/24600/1/src/osmo-bsc/handover_ctrl.c@98">Patch Set #1, Line 98:</a> <code style="font-family:monospace,monospace"> if (strcmp(cmd->value, "default") == 0) \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(before '\' should be space, not tab)</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/+/24600/1/src/osmo-bsc/handover_ctrl.c@114">Patch Set #1, Line 114:</a> <code style="font-family:monospace,monospace">#undef HO_CFG_ONE_MEMBER</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">please insert spacing here. The above two lines belong with the HO_CFG_ONE_MEMBER() definition above.<br>Below CTRL_CMD_DEFINE is unrelated.</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/+/24600/1/src/osmo-bsc/handover_ctrl.c@161">Patch Set #1, Line 161:</a> <code style="font-family:monospace,monospace"> rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_##NAME);\</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(usually no '\' on last line; if you want to keep it maybe add a space before it)</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/+/24600/1/src/osmo-bsc/handover_ctrl.c@163">Patch Set #1, Line 163:</a> <code style="font-family:monospace,monospace">HO_CFG_ALL_MEMBERS</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">For improved readability it may make sense to move HO_CFG_ALL_MEMBERS at the end of the list here. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">@pespin i don't understand what you mean</p><p style="white-space: pre-wrap; word-wrap: break-word;">it looks fine to me: defines a local impl of HO_CFG_ONE_MEMBER, then "calls" HO_CFG_ALL_MEMBERS to apply the HO_CFG_ONE_MEMBER definition to each ho cfg item, then undef HO_CFG_ONE_MEMBER again.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/24600">change 24600</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/+/24600"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I12f143906818fd6b16e8783157cbb1eb51e49ffc </div>
<div style="display:none"> Gerrit-Change-Number: 24600 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Assignee: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 14 Jun 2021 15:46:16 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>