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

[Neels Hofmeyr]

Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11552 )

Change subject: LCLS: add bts-loop variant
......................................................................


Patch Set 3: Code-Review-1

(8 comments)

https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c
File src/osmo-bsc/osmo_bsc_lcls.c:

https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c@238
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 */
    ...
  }

)


https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c@240
PS3, Line 240: 	if (conn->lcls.other) {
(may I suggest early-exit coding style...

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

  normal case

)


https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c@243
PS3, Line 243: 		LOGPFSM(conn->lcls.fi, "LCLS conn other is unavailable for %s!\n", enable ? "enable" : "disable");
hmm, maybe: "%s LCLS: other conn is not available\n"


https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c@244
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?


https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c@247
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?


https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c@295
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.


https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_lcls.c@648
PS3, Line 648: 		lcls_rsl(conn, true);
switch()


https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_vty.c
File src/osmo-bsc/osmo_bsc_vty.c:

https://gerrit.osmocom.org/#/c/11552/3/src/osmo-bsc/osmo_bsc_vty.c@653
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 https://gerrit.osmocom.org/11552
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

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 sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
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: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181105/5b3ca7de/attachment.htm>