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-smlc/+/20470 )
Change subject: initial working osmo-smlc implementation
......................................................................
Patch Set 7:
(13 comments)
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/include/osmocom/smlc/sccp_lb_inst.h
File include/osmocom/smlc/sccp_lb_inst.h:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/include/osmocom/smlc/sccp_lb_inst.h@52
PS6, Line 52: Compatibility with legacy osmo-hnbgw that was unable to properly handle RESET messages. Set to 'false' to
: * require proper RESET procedures, set to 'true' to implicitly put a lb_peer in RAN_PEER_ST_READY upon the
: * first CO message. Default is false = be strict. */
> we are not talking to osmo-hnbgw. […]
copy paste artifact, this should be dropped here indeed
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c
File src/osmo-smlc/cell_locations.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@38
PS6, Line 38: uint32_t
> Do we really need to enforce 'uint32_t' here? The maximum would be 63 * 550 == 34650, so 'uint16_t' […]
there is also Access Delay being the TA expanded to 8 bit range, 255 * 550 fits in 18 bits
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@194
PS6, Line 194: DEFUN
> Would be nice to attach 'when-it-applies' attributes to all configuration commands.
i have not looked at how that works, could you help me with that, in a subsequent patch?
The cell locations config takes effect immediately.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/cell_locations.c@248
PS6, Line 248: unidirectional
> You probably mean omnidirectional here. "unidirectional" would be the opposite.
omni!
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c
File src/osmo-smlc/lb_conn.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c@49
PS6, Line 49: if (total == 0
> No need for a line break.
i find that it better illustrates the two cases for 'total'
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_conn.c@78
PS6, Line 78: use_count
> Hmm, so this doesn't compile?
not sure what i was thinking, totally fine
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c
File src/osmo-smlc/lb_peer.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@172
PS6, Line 172: struct msgb *msg = ctx->msg
> Who is responsible for free()ing this msgb? The sender of a event?
thanks for reminding me, i was leaking msgb in two places. this time i verified it by running ttcn tests and looking at the msgb count
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/lb_peer.c@463
PS6, Line 463: " Dropping message.\n");
> is there some kind of STATUS we should return to the peer?
i have no idea, really, could you tell me what should happen, or should i try to find out?
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_data.c
File src/osmo-smlc/smlc_data.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_data.c@27
PS6, Line 27: smlc_ctr_description
> I find it hard to read, let's use line wraps please.
you marked "smlc_ctr_description", but you mean the strings below, right?
(the single line nature comes from using a '%s/...' rule)
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c
File src/osmo-smlc/smlc_loc_req.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_loc_req.c@152
PS6, Line 152: smlc_loc_req = (struct smlc_loc_req){
> This could be twice shorter: […]
because i don't know whether the future adds more elements to the struct, and this zero initializes all members that i don't set. Let's not micro optimise and rather opt for maintainability and non-randomness.
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_main.c
File src/osmo-smlc/smlc_main.c:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/src/osmo-smlc/smlc_main.c@179
PS6, Line 179: loglevel = LOGL_NOTICE
> AFAIR, it's safe to omit this filed: libosmocore would apply LOGL_NOTICE automatically.
i just copy pasted it from somewhere
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/cell_locations.vty
File tests/cell_locations.vty:
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/cell_locations.vty@83
PS6, Line 83: logging level llapd notice
: logging level linp notice
: logging level lmux notice
: logging level lmi notice
: logging level lmib notice
: logging level lsms notice
> This shall not be a part of the transcript test: adding new logging categories to libosmocore would […]
whoa this is an accident, thanks
https://gerrit.osmocom.org/c/osmo-smlc/+/20470/6/tests/osmo-smlc.cfg
File tests/osmo-smlc.cfg:
PS6:
> Empty config? Why not just do: […]
because it's annoying to care about build-time files, and maybe some day we want some actual config in here
--
To view, visit https://gerrit.osmocom.org/c/osmo-smlc/+/20470
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-smlc
Gerrit-Branch: master
Gerrit-Change-Id: I917ba8fc51a1f1150be77ae01e12a7b16a853052
Gerrit-Change-Number: 20470
Gerrit-PatchSet: 7
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: Vadim Yanitskiy <vyanitskiy at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Tue, 13 Oct 2020 02:45:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Comment-In-Reply-To: Vadim Yanitskiy <vyanitskiy at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201013/53bb2b18/attachment.htm>