Change in osmo-bsc[master]: handover_ctrl: add control interface for handover settings
gerrit-no-reply at lists.osmocom.org
Mon Jun 14 15:46:16 UTC 2021
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/24600 )
Change subject: handover_ctrl: add control interface for handover settings
Patch Set 1:
PS1, Line 41: |handover_ho_active|RW|No|"0","1","default"|Enable/disable handover.
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)
So to be complete there should also be bts.N.handover* nodes
PS1, Line 33: bool verify_vty_cmd_arg(void *ctx, const char *range, const char *value)
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.
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().
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?
PS1, Line 80: CTRL_CMD_DEFINE(NAME, "handover_"#NAME); \
The vty command name is in VTY_CMD;
NAME is the name used in C code.
The difference by some examples:
NAME VTY0 VTY_CMD
algorithm "" handover algorithm
hodec1_pwr_interval handover1 power budget interval
hodec2_rxlev_avg_win handover2 window rxlev averaging
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.
However what I don't like that much is that now the naming on the CTRL is quite different from on the VTY.
The discrepancy in naming I think came from legacy code and it was acceptable as long as it was hidden in the code.
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.
I see these ways:
1) adjust the #NAME strings to more exactly match the VTY command string.
for that we need to also replace the strings in the handover C code (where osmo-bsc queries the config).
2) add another argument to HO_CFG_ONE_MEMBER that is exactly the CTRL command.
I prefer 1).
Also I would like to see scoping as: handover.* , handover1.* and handover2.*
So for example, instead of
I would like to see
PS1, Line 88: cmd->reply = talloc_asprintf(cmd, VTY_WRITE_FMT, VTY_WRITE_CONV(val)); \
> why are we using VTY stuff here? Are these fmts safe for ctrl interface? […]
the HO_CFG_ONE_MEMBER() has arguments called VTY_WRITE_FMT and VTY_WRITE_CONV.
That's because the macro scheme was invented to avoid code dup and copy paste errors in the handover VTY configuration.
Now the same are being used for CTRL.
We maybe could rename the macro arguments to be more general, like just drop all the VTY_ prefixes across handover_cfg.*?
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
PS1, Line 98: if (strcmp(cmd->value, "default") == 0) \
(before '\' should be space, not tab)
PS1, Line 114: #undef HO_CFG_ONE_MEMBER
please insert spacing here. The above two lines belong with the HO_CFG_ONE_MEMBER() definition above.
Below CTRL_CMD_DEFINE is unrelated.
PS1, Line 161: rc |= ctrl_cmd_install(CTRL_NODE_ROOT, &cmd_##NAME);\
(usually no '\' on last line; if you want to keep it maybe add a space before it)
PS1, Line 163: HO_CFG_ALL_MEMBERS
> For improved readability it may make sense to move HO_CFG_ALL_MEMBERS at the end of the list here. […]
@pespin i don't understand what you mean
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.
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/24600
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Assignee: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 14 Jun 2021 15:46:16 +0000
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the gerrit-log