Change in osmo-bsc[master]: handover_ctrl: add control interface for handover settings

neels 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:

(8 comments)

https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/doc/manuals/chapters/control.adoc 
File doc/manuals/chapters/control.adoc:

https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/doc/manuals/chapters/control.adoc@41 
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


https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c 
File src/osmo-bsc/handover_ctrl.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@33 
PS1, Line 33: bool verify_vty_cmd_arg(void *ctx, const char *range, const char *value)
> Ack
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?


https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@80 
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

 handover_hodec2_rxlev_avg_win

I would like to see

 handover2.window_rxlev_averaging
 bts.N.handover2.window_rxlev_averaging


https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@88 
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


https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@98 
PS1, Line 98: 	if (strcmp(cmd->value, "default") == 0)	\
(before '\' should be space, not tab)


https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@114 
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.


https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@161 
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)


https://gerrit.osmocom.org/c/osmo-bsc/+/24600/1/src/osmo-bsc/handover_ctrl.c@163 
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-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I12f143906818fd6b16e8783157cbb1eb51e49ffc
Gerrit-Change-Number: 24600
Gerrit-PatchSet: 1
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
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210614/c4c133bd/attachment.htm>


More information about the gerrit-log mailing list