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

[neels]

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 7: Code-Review+1

(2 comments)

nice one, patch became much smaller!

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

https://gerrit.osmocom.org/c/osmo-bsc/+/24600/6/src/osmo-bsc/handover_ctrl.c@73 
PS6, Line 73: CTRL_CMD_DEFINE(NAME, CTRL_CMD VTY_CMD_PREFIX VTY_CMD); \
> When CTRL_CMD it is used the constant will be defined like "handover2 min-free-slots tch-h\0", VTY_C […]
I figured out your nifty trick there later on  : )
interesting solution, even though it had most of us confused at first.
with a comment explaining in detail that could have passed CR too : )


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

https://gerrit.osmocom.org/c/osmo-bsc/+/24600/7/src/osmo-bsc/handover_ctrl.c@185 
PS7, Line 185: {
0) I don't understand yet where the spaces-to-dots conversion happens.
Is it possible that that step also sanitizes the '/' characters already,
and we could get away without needing a filter function?? (I guess not)

1) Let's avoid allocating most strings with this step:

  if (osmo_separated_identifiers_valid(cmd->name, " -"))
      return cmd;

2) I thought we could also try to use the existing osmo_identifier_sanitize_buf() API, something like

  cmd->name = talloc_strdup(cmd->name);
  osmo_identifier_sanitize_buf(cmd->name, NULL, "-");

But AFAICT there are a number of problems with that, because unfortunately sep_chars has opposite meaning in osmo_separated_identifiers_valid() and osmo_identifier_sanitize_buf().
This loop here is also fine I guess, because there is only fixed hardcoded input data anyway. But if you like maybe give it a try...



-- 
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: 7
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Assignee: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
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: Wed, 07 Jul 2021 21:31:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: neels <nhofmeyr at sysmocom.de>
Comment-In-Reply-To: dexter <pmaier at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210707/0b72fe32/attachment.htm>