Change in osmo-bsc[master]: LCLS: add bts-loop variant

Neels Hofmeyr gerrit-no-reply at
Mon Nov 5 03:24:57 UTC 2018

Neels Hofmeyr has posted comments on this change. ( )

Change subject: LCLS: add bts-loop variant

Patch Set 3: Code-Review-1

File src/osmo-bsc/osmo_bsc_lcls.c:
PS3, Line 238: 	uint16_t port = enable ? conn->lcls.other->lchan->abis_ip.bound_port : lchan->abis_ip.connect_port;
(may I suggest comments...

  if (enable) {
    /* redirect directly to remote BTS's IP:port */
  } else {
    /* redirect back to the MGW's IP:port */

PS3, Line 240: 	if (conn->lcls.other) {
(may I suggest early-exit coding style...

  if (!conn->lcls.other) {
    err handling

  normal case

PS3, Line 243: 		LOGPFSM(conn->, "LCLS conn other is unavailable for %s!\n", enable ? "enable" : "disable");
hmm, maybe: "%s LCLS: other conn is not available\n"
PS3, Line 244: 		return;
is there no error handling? what happens if this fails, the LCLS state thinks the loop is closed but it actually isn't?
PS3, Line 247: 	abis_rsl_sendmsg(msg);
evaluate rc and roll back LCLS state on error? Or even tear down conn if we can't send RSL?
PS3, Line 295: 		lcls_rsl(conn, false);
I would prefer a full-on switch() statement, making sure we cover all enum values, and catch wrongly initialized values.
PS3, Line 648: 		lcls_rsl(conn, true);
File src/osmo-bsc/osmo_bsc_vty.c:
PS3, Line 653:       "Enable LCLS with loopping traffic between BTS\n")
"looping". In this case I'd be fine to fix the typo in the previous code at the same time.

To view, visit
To unsubscribe, or for help writing mail filters, visit

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e8379f31037f2c48da69a01919701919a3066a2
Gerrit-Change-Number: 11552
Gerrit-PatchSet: 3
Gerrit-Owner: Max <msuraev at>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at>
Gerrit-CC: Harald Welte <laforge at>
Gerrit-Comment-Date: Mon, 05 Nov 2018 03:24:57 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list