This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
neels gerrit-no-reply at lists.osmocom.orgneels 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/ab52d76f/attachment.htm>